[PM-16098] Improved cipher decryption error handling (#12468)

* [PM-16098] Add decryptionFailure flag to CipherView * [PM-16098] Add failedToDecryptCiphers$ observable to CipherService * [PM-16098] Introduce decryption-failure-dialog.component * [PM-16098] Disable cipher rows for the Web Vault * [PM-16098] Show decryption error dialog on vault load or when attempting to view/edit a corrupted cipher * [PM-16098] Browser - Show decryption error dialog on vault load or when attempting to view/edit a corrupted cipher * [PM-16098] Desktop - Show decryption error dialog on vault load or when attempting to view a corrupted cipher. Remove edit/clone context menu options and footer actions. * [PM-16098] Add CS link to decryption failure dialog * [PM-16098] Return cipherViews and move filtering of isDeleted to consumers * [PM-16098] Throw an error when retrieving cipher data for key rotation when a decryption failure is present * [PM-16098] Properly filter out deleted, corrupted ciphers when showing dialog within the Vault * [PM-16098] Show the decryption error dialog when attempting to view a cipher in trash and disable the restore option * [PM-16098] Exclude failed to decrypt ciphers from getAllDecrypted method and cipherViews$ observable * [PM-16098] Avoid re-sorting remainingCiphers$ as it was redundant * [PM-16098] Update tests * [PM-16098] Prevent opening view dialog in AC for corrupted ciphers * [PM-16098] Remove withLatestFrom operator that was causing race conditions when navigating away from the individual vault * [PM-16098] Ensure decryption error dialog is only shown once on Desktop when switching accounts
bitwarden · Jan 8, 2025 · d72dd2e · d72dd2e
1 parent 65a27e7
commit d72dd2e
Show file tree

Hide file tree

Showing 29 changed files with 467 additions and 74 deletions.
diff --git a/apps/browser/src/_locales/en/messages.json b/apps/browser/src/_locales/en/messages.json
@@ -2804,6 +2804,20 @@
   "error": {
     "message": "Error"
   },
+  "decryptionError": {
+    "message": "Decryption error"
+  },
+  "couldNotDecryptVaultItemsBelow": {
+    "message": "Bitwarden could not decrypt the vault item(s) listed below."
+  },
+  "contactCSToAvoidDataLossPart1": {
+    "message": "Contact customer success",
+    "description": "This is part of a larger sentence. The full sentence will read 'Contact customer success to avoid additional data loss.'"
+  },
+  "contactCSToAvoidDataLossPart2": {
+    "message": "to avoid additional data loss.",
+    "description": "This is part of a larger sentence. The full sentence will read 'Contact customer success to avoid additional data loss.'"
+  },
   "generateUsername": {
     "message": "Generate username"
   },

diff --git a/...er/src/vault/popup/components/vault-v2/item-more-options/item-more-options.component.html b/...er/src/vault/popup/components/vault-v2/item-more-options/item-more-options.component.html
@@ -5,6 +5,7 @@
     size="small"
     [attr.aria-label]="'moreOptionsLabel' | i18n: cipher.name"
     [title]="'moreOptionsTitle' | i18n: cipher.name"
+    [disabled]="cipher.decryptionFailure"
     [bitMenuTriggerFor]="moreOptions"
   ></button>
   <bit-menu #moreOptions>

diff --git a/...up/components/vault-v2/vault-list-items-container/vault-list-items-container.component.ts b/...up/components/vault-v2/vault-list-items-container/vault-list-items-container.component.ts
@@ -18,19 +18,25 @@ import { map } from "rxjs";
 import { JslibModule } from "@bitwarden/angular/jslib.module";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
 import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
+import { CipherId } from "@bitwarden/common/types/guid";
 import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
 import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
 import {
   BadgeModule,
   ButtonModule,
   CompactModeService,
+  DialogService,
   IconButtonModule,
   ItemModule,
   SectionComponent,
   SectionHeaderComponent,
   TypographyModule,
 } from "@bitwarden/components";
-import { OrgIconDirective, PasswordRepromptService } from "@bitwarden/vault";
+import {
+  DecryptionFailureDialogComponent,
+  OrgIconDirective,
+  PasswordRepromptService,
+} from "@bitwarden/vault";
 
 import { BrowserApi } from "../../../../../platform/browser/browser-api";
 import BrowserPopupUtils from "../../../../../platform/popup/browser-popup-utils";
@@ -55,6 +61,7 @@ import { ItemMoreOptionsComponent } from "../item-more-options/item-more-options
     ItemMoreOptionsComponent,
     OrgIconDirective,
     ScrollingModule,
+    DecryptionFailureDialogComponent,
   ],
   selector: "app-vault-list-items-container",
   templateUrl: "vault-list-items-container.component.html",
@@ -158,6 +165,7 @@ export class VaultListItemsContainerComponent implements AfterViewInit {
     private cipherService: CipherService,
     private router: Router,
     private platformUtilsService: PlatformUtilsService,
+    private dialogService: DialogService,
   ) {}
 
   async ngAfterViewInit() {
@@ -209,6 +217,13 @@ export class VaultListItemsContainerComponent implements AfterViewInit {
     this.viewCipherTimeout = window.setTimeout(
       async () => {
         try {
+          if (cipher.decryptionFailure) {
+            DecryptionFailureDialogComponent.open(this.dialogService, {
+              cipherIds: [cipher.id as CipherId],
+            });
+            return;
+          }
+
           const repromptPassed = await this.passwordRepromptService.passwordRepromptCheck(cipher);
           if (!repromptPassed) {
             return;

diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-v2.component.ts
@@ -1,15 +1,17 @@
 import { ScrollingModule } from "@angular/cdk/scrolling";
 import { CommonModule } from "@angular/common";
-import { Component, OnDestroy, OnInit } from "@angular/core";
+import { Component, DestroyRef, OnDestroy, OnInit } from "@angular/core";
 import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
 import { RouterLink } from "@angular/router";
 import { combineLatest, Observable, shareReplay, switchMap } from "rxjs";
+import { filter, map, take } from "rxjs/operators";
 
 import { JslibModule } from "@bitwarden/angular/jslib.module";
-import { CollectionId, OrganizationId } from "@bitwarden/common/types/guid";
+import { CipherId, CollectionId, OrganizationId } from "@bitwarden/common/types/guid";
+import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
 import { CipherType } from "@bitwarden/common/vault/enums";
-import { ButtonModule, Icons, NoItemsModule } from "@bitwarden/components";
-import { VaultIcons } from "@bitwarden/vault";
+import { ButtonModule, DialogService, Icons, NoItemsModule } from "@bitwarden/components";
+import { DecryptionFailureDialogComponent, VaultIcons } from "@bitwarden/vault";
 
 import { CurrentAccountComponent } from "../../../../auth/popup/account-switching/current-account.component";
 import { PopOutComponent } from "../../../../platform/popup/components/pop-out.component";
@@ -52,6 +54,7 @@ enum VaultState {
     NewItemDropdownV2Component,
     ScrollingModule,
     VaultHeaderV2Component,
+    DecryptionFailureDialogComponent,
   ],
   providers: [VaultUiOnboardingService],
 })
@@ -89,6 +92,9 @@ export class VaultV2Component implements OnInit, OnDestroy {
     private vaultPopupItemsService: VaultPopupItemsService,
     private vaultPopupListFiltersService: VaultPopupListFiltersService,
     private vaultUiOnboardingService: VaultUiOnboardingService,
+    private destroyRef: DestroyRef,
+    private cipherService: CipherService,
+    private dialogService: DialogService,
   ) {
     combineLatest([
       this.vaultPopupItemsService.emptyVault$,
@@ -116,6 +122,19 @@ export class VaultV2Component implements OnInit, OnDestroy {
 
   async ngOnInit() {
     await this.vaultUiOnboardingService.showOnboardingDialog();
+
+    this.cipherService.failedToDecryptCiphers$
+      .pipe(
+        map((ciphers) => ciphers.filter((c) => !c.isDeleted)),
+        filter((ciphers) => ciphers.length > 0),
+        take(1),
+        takeUntilDestroyed(this.destroyRef),
+      )
+      .subscribe((ciphers) => {
+        DecryptionFailureDialogComponent.open(this.dialogService, {
+          cipherIds: ciphers.map((c) => c.id as CipherId),
+        });
+      });
   }
 
   ngOnDestroy(): void {}

diff --git a/apps/browser/src/vault/popup/services/vault-popup-items.service.spec.ts b/apps/browser/src/vault/popup/services/vault-popup-items.service.spec.ts
@@ -58,6 +58,7 @@ describe("VaultPopupItemsService", () => {
     cipherServiceMock.getAllDecrypted.mockResolvedValue(cipherList);
     cipherServiceMock.ciphers$ = new BehaviorSubject(null);
     cipherServiceMock.localData$ = new BehaviorSubject(null);
+    cipherServiceMock.failedToDecryptCiphers$ = new BehaviorSubject([]);
     searchService.searchCiphers.mockImplementation(async (_, __, ciphers) => ciphers);
     cipherServiceMock.filterCiphersForUrl.mockImplementation(async (ciphers) =>
       ciphers.filter((c) => ["0", "1"].includes(c.id)),
@@ -294,21 +295,6 @@ describe("VaultPopupItemsService", () => {
       });
     });
 
-    it("should sort by last used then by name by default", (done) => {
-      service.remainingCiphers$.subscribe(() => {
-        expect(cipherServiceMock.getLocaleSortingFunction).toHaveBeenCalled();
-        done();
-      });
-    });
-
-    it("should NOT sort by last used then by name when search text is applied", (done) => {
-      service.applyFilter("Login");
-      service.remainingCiphers$.subscribe(() => {
-        expect(cipherServiceMock.getLocaleSortingFunction).not.toHaveBeenCalled();
-        done();
-      });
-    });
-
     it("should filter remainingCiphers$ down to search term", (done) => {
       const cipherList = Object.values(allCiphers);
       const searchText = "Login";

diff --git a/apps/browser/src/vault/popup/services/vault-popup-items.service.ts b/apps/browser/src/vault/popup/services/vault-popup-items.service.ts
@@ -90,6 +90,8 @@ export class VaultPopupItemsService {
     tap(() => this._ciphersLoading$.next()),
     waitUntilSync(this.syncService),
     switchMap(() => Utils.asyncToObservable(() => this.cipherService.getAllDecrypted())),
+    withLatestFrom(this.cipherService.failedToDecryptCiphers$),
+    map(([ciphers, failedToDecryptCiphers]) => [...failedToDecryptCiphers, ...ciphers]),
     shareReplay({ refCount: true, bufferSize: 1 }),
   );
 
@@ -190,11 +192,6 @@ export class VaultPopupItemsService {
         (cipher) => !autoFillCiphers.includes(cipher) && !favoriteCiphers.includes(cipher),
       ),
     ),
-    withLatestFrom(this._hasSearchText$),
-    map(([ciphers, hasSearchText]) =>
-      // Do not sort alphabetically when there is search text, default to the search service scoring
-      hasSearchText ? ciphers : ciphers.sort(this.cipherService.getLocaleSortingFunction()),
-    ),
     shareReplay({ refCount: false, bufferSize: 1 }),
   );
 

diff --git a/...vault/popup/settings/trash-list-items-container/trash-list-items-container.component.html b/...vault/popup/settings/trash-list-items-container/trash-list-items-container.component.html
@@ -27,7 +27,12 @@ <h2 bitTypography="h6">
             [bitMenuTriggerFor]="moreOptions"
           ></button>
           <bit-menu #moreOptions>
-            <button type="button" bitMenuItem (click)="restore(cipher)">
+            <button
+              type="button"
+              bitMenuItem
+              (click)="restore(cipher)"
+              *ngIf="!cipher.decryptionFailure"
+            >
               {{ "restore" | i18n }}
             </button>
             <button type="button" bitMenuItem *appCanDeleteCipher="cipher" (click)="delete(cipher)">

diff --git a/...c/vault/popup/settings/trash-list-items-container/trash-list-items-container.component.ts b/...c/vault/popup/settings/trash-list-items-container/trash-list-items-container.component.ts
@@ -7,6 +7,7 @@ import { Router } from "@angular/router";
 import { JslibModule } from "@bitwarden/angular/jslib.module";
 import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
 import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
+import { CipherId } from "@bitwarden/common/types/guid";
 import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
 import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
 import {
@@ -19,7 +20,11 @@ import {
   ToastService,
   TypographyModule,
 } from "@bitwarden/components";
-import { CanDeleteCipherDirective, PasswordRepromptService } from "@bitwarden/vault";
+import {
+  CanDeleteCipherDirective,
+  DecryptionFailureDialogComponent,
+  PasswordRepromptService,
+} from "@bitwarden/vault";
 
 @Component({
   selector: "app-trash-list-items-container",
@@ -35,6 +40,7 @@ import { CanDeleteCipherDirective, PasswordRepromptService } from "@bitwarden/va
     MenuModule,
     IconButtonModule,
     TypographyModule,
+    DecryptionFailureDialogComponent,
   ],
   changeDetection: ChangeDetectionStrategy.OnPush,
 })
@@ -105,6 +111,13 @@ export class TrashListItemsContainerComponent {
   }
 
   async onViewCipher(cipher: CipherView) {
+    if (cipher.decryptionFailure) {
+      DecryptionFailureDialogComponent.open(this.dialogService, {
+        cipherIds: [cipher.id as CipherId],
+      });
+      return;
+    }
+
     const repromptPassed = await this.passwordRepromptService.passwordRepromptCheck(cipher);
     if (!repromptPassed) {
       return;

diff --git a/apps/desktop/src/app/app.module.ts b/apps/desktop/src/app/app.module.ts
@@ -7,7 +7,8 @@ import { NgModule } from "@angular/core";
 
 import { ColorPasswordCountPipe } from "@bitwarden/angular/pipes/color-password-count.pipe";
 import { ColorPasswordPipe } from "@bitwarden/angular/pipes/color-password.pipe";
-import { DialogModule, CalloutModule } from "@bitwarden/components";
+import { CalloutModule, DialogModule } from "@bitwarden/components";
+import { DecryptionFailureDialogComponent } from "@bitwarden/vault";
 
 import { AccessibilityCookieComponent } from "../auth/accessibility-cookie.component";
 import { DeleteAccountComponent } from "../auth/delete-account.component";
@@ -61,6 +62,7 @@ import { SendComponent } from "./tools/send/send.component";
     CalloutModule,
     DeleteAccountComponent,
     UserVerificationComponent,
+    DecryptionFailureDialogComponent,
   ],
   declarations: [
     AccessibilityCookieComponent,

diff --git a/apps/desktop/src/locales/en/messages.json b/apps/desktop/src/locales/en/messages.json
@@ -249,6 +249,20 @@
   "error": {
     "message": "Error"
   },
+  "decryptionError": {
+    "message": "Decryption error"
+  },
+  "couldNotDecryptVaultItemsBelow": {
+    "message": "Bitwarden could not decrypt the vault item(s) listed below."
+  },
+  "contactCSToAvoidDataLossPart1": {
+    "message": "Contact customer success",
+    "description": "This is part of a larger sentence. The full sentence will read 'Contact customer success to avoid additional data loss.'"
+  },
+  "contactCSToAvoidDataLossPart2": {
+    "message": "to avoid additional data loss.",
+    "description": "This is part of a larger sentence. The full sentence will read 'Contact customer success to avoid additional data loss.'"
+  },
   "january": {
     "message": "January"
   },

diff --git a/apps/desktop/src/vault/app/vault/vault.component.ts b/apps/desktop/src/vault/app/vault/vault.component.ts
@@ -10,8 +10,8 @@ import {
   ViewContainerRef,
 } from "@angular/core";
 import { ActivatedRoute, Router } from "@angular/router";
-import { Subject, takeUntil, switchMap } from "rxjs";
-import { first } from "rxjs/operators";
+import { combineLatest, firstValueFrom, Subject, takeUntil, switchMap } from "rxjs";
+import { filter, first, map, take } from "rxjs/operators";
 
 import { ModalRef } from "@bitwarden/angular/components/modal/modal.ref";
 import { ModalService } from "@bitwarden/angular/services/modal.service";
@@ -28,13 +28,15 @@ import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.servic
 import { MessagingService } from "@bitwarden/common/platform/abstractions/messaging.service";
 import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
 import { SyncService } from "@bitwarden/common/platform/sync";
+import { CipherId } from "@bitwarden/common/types/guid";
+import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service";
 import { TotpService } from "@bitwarden/common/vault/abstractions/totp.service";
 import { CipherType } from "@bitwarden/common/vault/enums";
 import { CipherRepromptType } from "@bitwarden/common/vault/enums/cipher-reprompt-type";
 import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view";
 import { FolderView } from "@bitwarden/common/vault/models/view/folder.view";
 import { DialogService } from "@bitwarden/components";
-import { PasswordRepromptService } from "@bitwarden/vault";
+import { DecryptionFailureDialogComponent, PasswordRepromptService } from "@bitwarden/vault";
 
 import { SearchBarService } from "../../../app/layout/search/search-bar.service";
 import { GeneratorComponent } from "../../../app/tools/generator.component";
@@ -113,6 +115,7 @@ export class VaultComponent implements OnInit, OnDestroy {
     private billingAccountProfileStateService: BillingAccountProfileStateService,
     private configService: ConfigService,
     private accountService: AccountService,
+    private cipherService: CipherService,
   ) {}
 
   async ngOnInit() {
@@ -238,6 +241,25 @@ export class VaultComponent implements OnInit, OnDestroy {
         notificationId: authRequest.id,
       });
     }
+
+    // Store a reference to the current active account during page init
+    const activeAccount = await firstValueFrom(this.accountService.activeAccount$);
+
+    // Combine with the activeAccount$ to ensure we only show the dialog for the current account from ngOnInit.
+    // The account switching process updates the cipherService before Vault is destroyed and would cause duplicate emissions
+    combineLatest([this.accountService.activeAccount$, this.cipherService.failedToDecryptCiphers$])
+      .pipe(
+        filter(([account]) => account.id === activeAccount.id),
+        map(([_, ciphers]) => ciphers.filter((c) => !c.isDeleted)),
+        filter((ciphers) => ciphers.length > 0),
+        take(1),
+        takeUntil(this.componentIsDestroyed$),
+      )
+      .subscribe((ciphers) => {
+        DecryptionFailureDialogComponent.open(this.dialogService, {
+          cipherIds: ciphers.map((c) => c.id as CipherId),
+        });
+      });
   }
 
   ngOnDestroy() {
@@ -302,6 +324,12 @@ export class VaultComponent implements OnInit, OnDestroy {
           }),
       },
     ];
+
+    if (cipher.decryptionFailure) {
+      invokeMenu(menu);
+      return;
+    }
+
     if (!cipher.isDeleted) {
       menu.push({
         label: this.i18nService.t("edit"),