Prevented double decryption (#17768)

libs/common/src/vault/abstractions/cipher-encryption.service.ts

@@ -67,7 +67,10 @@ export abstract class CipherEncryptionService {
    *
    * @returns A promise that resolves to an array of decrypted cipher views
    */
-  abstract decryptManyLegacy(ciphers: Cipher[], userId: UserId): Promise<CipherView[]>;
+  abstract decryptManyLegacy(
+    ciphers: Cipher[],
+    userId: UserId,
+  ): Promise<[CipherView[], CipherView[]]>;
   /**
    * Decrypts many ciphers using the SDK for the given userId, and returns a list of
    * failures.

libs/common/src/vault/services/cipher.service.spec.ts

@@ -807,7 +807,7 @@ describe("Cipher Service", () => {
 
       // Set up expected results
       const expectedSuccessCipherViews = [
-        { id: mockCiphers[0].id, name: "Success 1" } as unknown as CipherListView,
+        { id: mockCiphers[0].id, name: "Success 1", decryptionFailure: false } as CipherView,
       ];
 
       const expectedFailedCipher = new CipherView(mockCiphers[1]);
@@ -815,6 +815,11 @@ describe("Cipher Service", () => {
       expectedFailedCipher.decryptionFailure = true;
       const expectedFailedCipherViews = [expectedFailedCipher];
 
+      cipherEncryptionService.decryptManyLegacy.mockResolvedValue([
+        expectedSuccessCipherViews,
+        expectedFailedCipherViews,
+      ]);
+
       // Execute
       const [successes, failures] = await (cipherService as any).decryptCiphers(
         mockCiphers,
@@ -822,10 +827,7 @@ describe("Cipher Service", () => {
       );
 
       // Verify the SDK was used for decryption
-      expect(cipherEncryptionService.decryptManyWithFailures).toHaveBeenCalledWith(
-        mockCiphers,
-        userId,
-      );
+      expect(cipherEncryptionService.decryptManyLegacy).toHaveBeenCalledWith(mockCiphers, userId);
 
       expect(successes).toEqual(expectedSuccessCipherViews);
       expect(failures).toEqual(expectedFailedCipherViews);

libs/common/src/vault/services/cipher.service.ts

@@ -2143,15 +2143,19 @@ export class CipherService implements CipherServiceAbstraction {
     userId: UserId,
     fullDecryption: boolean = true,
   ): Promise<[CipherViewLike[], CipherView[]]> {
+    if (fullDecryption) {
+      const [decryptedViews, failedViews] = await this.cipherEncryptionService.decryptManyLegacy(
+        ciphers,
+        userId,
+      );
+      return [decryptedViews.sort(this.getLocaleSortingFunction()), failedViews];
+    }
+
     const [decrypted, failures] = await this.cipherEncryptionService.decryptManyWithFailures(
       ciphers,
       userId,
     );
 
-    const decryptedViews = fullDecryption
-      ? await Promise.all(decrypted.map((c) => this.getFullCipherView(c)))
-      : decrypted;
-
     const failedViews = failures.map((c) => {
       const cipher_view = new CipherView(c);
       cipher_view.name = "[error: cannot decrypt]";
@@ -2159,7 +2163,7 @@ export class CipherService implements CipherServiceAbstraction {
       return cipher_view;
     });
 
-    return [decryptedViews.sort(this.getLocaleSortingFunction()), failedViews];
+    return [decrypted.sort(this.getLocaleSortingFunction()), failedViews];
   }
 
   /** Fetches the full `CipherView` when a `CipherListView` is passed. */

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

@@ -496,9 +496,11 @@ describe("DefaultCipherEncryptionService", () => {
         .mockReturnValueOnce(expectedViews[0])
         .mockReturnValueOnce(expectedViews[1]);
 
-      const result = await cipherEncryptionService.decryptManyLegacy(ciphers, userId);
+      const [successfulDecryptions, failedDecryptions] =
+        await cipherEncryptionService.decryptManyLegacy(ciphers, userId);
 
-      expect(result).toEqual(expectedViews);
+      expect(successfulDecryptions).toEqual(expectedViews);
+      expect(failedDecryptions).toEqual([]);
       expect(mockSdkClient.vault().ciphers().decrypt).toHaveBeenCalledTimes(2);
       expect(CipherView.fromSdkCipherView).toHaveBeenCalledTimes(2);
     });

libs/common/src/vault/services/default-cipher-encryption.service.ts

@@ -168,7 +168,7 @@ export class DefaultCipherEncryptionService implements CipherEncryptionService {
     );
   }
 
-  decryptManyLegacy(ciphers: Cipher[], userId: UserId): Promise<CipherView[]> {
+  decryptManyLegacy(ciphers: Cipher[], userId: UserId): Promise<[CipherView[], CipherView[]]> {
     return firstValueFrom(
       this.sdkService.userClient$(userId).pipe(
         map((sdk) => {
@@ -178,38 +178,49 @@ export class DefaultCipherEncryptionService implements CipherEncryptionService {
 
           using ref = sdk.take();
 
-          return ciphers.map((cipher) => {
-            const sdkCipherView = ref.value.vault().ciphers().decrypt(cipher.toSdkCipher());
-            const clientCipherView = CipherView.fromSdkCipherView(sdkCipherView)!;
+          const successful: CipherView[] = [];
+          const failed: CipherView[] = [];
 
-            // Handle FIDO2 credentials if present
-            if (
-              clientCipherView.type === CipherType.Login &&
-              sdkCipherView.login?.fido2Credentials?.length
-            ) {
-              const fido2CredentialViews = ref.value
-                .vault()
-                .ciphers()
-                .decrypt_fido2_credentials(sdkCipherView);
+          ciphers.forEach((cipher) => {
+            try {
+              const sdkCipherView = ref.value.vault().ciphers().decrypt(cipher.toSdkCipher());
+              const clientCipherView = CipherView.fromSdkCipherView(sdkCipherView)!;
 
-              // TODO (PM-21259): Remove manual keyValue decryption for FIDO2 credentials.
-              // This is a temporary workaround until we can use the SDK for FIDO2 authentication.
-              const decryptedKeyValue = ref.value
-                .vault()
-                .ciphers()
-                .decrypt_fido2_private_key(sdkCipherView);
+              // Handle FIDO2 credentials if present
+              if (
+                clientCipherView.type === CipherType.Login &&
+                sdkCipherView.login?.fido2Credentials?.length
+              ) {
+                const fido2CredentialViews = ref.value
+                  .vault()
+                  .ciphers()
+                  .decrypt_fido2_credentials(sdkCipherView);
 
-              clientCipherView.login.fido2Credentials = fido2CredentialViews
-                .map((f) => {
-                  const view = Fido2CredentialView.fromSdkFido2CredentialView(f)!;
-                  view.keyValue = decryptedKeyValue;
-                  return view;
-                })
-                .filter((view): view is Fido2CredentialView => view !== undefined);
+                const decryptedKeyValue = ref.value
+                  .vault()
+                  .ciphers()
+                  .decrypt_fido2_private_key(sdkCipherView);
+
+                clientCipherView.login.fido2Credentials = fido2CredentialViews
+                  .map((f) => {
+                    const view = Fido2CredentialView.fromSdkFido2CredentialView(f)!;
+                    view.keyValue = decryptedKeyValue;
+                    return view;
+                  })
+                  .filter((view): view is Fido2CredentialView => view !== undefined);
+              }
+
+              successful.push(clientCipherView);
+            } catch (error) {
+              this.logService.error(`Failed to decrypt cipher ${cipher.id}: ${error}`);
+              const failedView = new CipherView(cipher);
+              failedView.name = "[error: cannot decrypt]";
+              failedView.decryptionFailure = true;
+              failed.push(failedView);
             }
-
-            return clientCipherView;
           });
+
+          return [successful, failed] as [CipherView[], CipherView[]];
         }),
         catchError((error: unknown) => {
           this.logService.error(`Failed to decrypt ciphers: ${error}`);