BIT-2355: Check to see if a cipher needs to be migrated when encrypting the cipher (#1442)

app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSource.kt

@@ -147,6 +147,10 @@ interface VaultSdkSource {
      *
      * This should only be called after a successful call to [initializeCrypto] for the associated
      * user.
+     *
+     * Note that this function will always add a [CipherView.key] to a cipher if it is missing,
+     * it is important to ensure that any [CipherView] being encrypted is pushed to the cloud if
+     * it was previously missing a `key` to ensure synchronization and prevent data-loss.
      */
     suspend fun encryptCipher(
         userId: String,

app/src/main/java/com/x8bit/bitwarden/data/vault/manager/CipherManagerImpl.kt

@@ -6,6 +6,7 @@ import com.bitwarden.core.Cipher
 import com.bitwarden.core.CipherView
 import com.x8bit.bitwarden.data.auth.datasource.disk.AuthDiskSource
 import com.x8bit.bitwarden.data.platform.util.asFailure
+import com.x8bit.bitwarden.data.platform.util.asSuccess
 import com.x8bit.bitwarden.data.platform.util.flatMap
 import com.x8bit.bitwarden.data.vault.datasource.disk.VaultDiskSource
 import com.x8bit.bitwarden.data.vault.datasource.network.model.AttachmentJsonRequest
@@ -105,20 +106,24 @@ class CipherManagerImpl(
         cipherView: CipherView,
     ): DeleteCipherResult {
         val userId = activeUserId ?: return DeleteCipherResult.Error
-        return ciphersService
+        return cipherView
-            .softDeleteCipher(cipherId = cipherId)
+            .encryptCipherAndCheckForMigration(userId = userId, cipherId = cipherId)
+            .flatMap { cipher ->
+                ciphersService
+                    .softDeleteCipher(cipherId = cipherId)
+                    .flatMap { vaultSdkSource.decryptCipher(userId = userId, cipher = cipher) }
+            }
+            .flatMap {
+                vaultSdkSource.encryptCipher(
+                    userId = userId,
+                    cipherView = it.copy(deletedDate = clock.instant()),
+                )
+            }
             .onSuccess {
-                vaultSdkSource
+                vaultDiskSource.saveCipher(
-                    .encryptCipher(
+                    userId = userId,
-                        userId = userId,
+                    cipher = it.toEncryptedNetworkCipherResponse(),
-                        cipherView = cipherView.copy(deletedDate = clock.instant()),
+                )
-                    )
-                    .onSuccess { cipher ->
-                        vaultDiskSource.saveCipher(
-                            userId = userId,
-                            cipher = cipher.toEncryptedNetworkCipherResponse(),
-                        )
-                    }
             }
             .fold(
                 onSuccess = { DeleteCipherResult.Success },
@@ -153,14 +158,13 @@ class CipherManagerImpl(
                 attachmentId = attachmentId,
             )
             .flatMap {
-                vaultSdkSource.encryptCipher(
+                cipherView
-                    userId = userId,
+                    .copy(
-                    cipherView = cipherView.copy(
                         attachments = cipherView.attachments?.mapNotNull {
                             if (it.id == attachmentId) null else it
                         },
-                    ),
+                    )
-                )
+                    .encryptCipherAndCheckForMigration(userId = userId, cipherId = cipherId)
             }
             .onSuccess { cipher ->
                 vaultDiskSource.saveCipher(
@@ -320,10 +324,10 @@ class CipherManagerImpl(
             fileName = fileName,
             key = null,
         )
-        return vaultSdkSource
+        return cipherView
-            .encryptCipher(
+            .encryptCipherAndCheckForMigration(
                 userId = userId,
-                cipherView = cipherView,
+                cipherId = requireNotNull(cipherView.id),
             )
             .flatMap { cipher ->
                 fileManager
@@ -400,10 +404,10 @@ class CipherManagerImpl(
     ): Result<File> {
         val userId = activeUserId ?: return IllegalStateException("No active user").asFailure()
 
-        val cipher = vaultSdkSource
+        val cipher = cipherView
-            .encryptCipher(
+            .encryptCipherAndCheckForMigration(
                 userId = userId,
-                cipherView = cipherView,
+                cipherId = requireNotNull(cipherView.id),
             )
             .fold(
                 onSuccess = { it },
@@ -443,4 +447,36 @@ class CipherManagerImpl(
             .onFailure { fileManager.delete(encryptedFile) }
             .map { decryptedFile }
     }
+
+    /**
+     * A helper method to check if the [CipherView] needs to be migrated when you encrypt it.
+     */
+    private suspend fun CipherView.encryptCipherAndCheckForMigration(
+        userId: String,
+        cipherId: String,
+    ): Result<Cipher> =
+        if (this.key == null) {
+            vaultSdkSource
+                .encryptCipher(userId = userId, cipherView = this)
+                .flatMap {
+                    ciphersService.updateCipher(
+                        cipherId = cipherId,
+                        body = it.toEncryptedNetworkCipher(),
+                    )
+                }
+                .flatMap { response ->
+                    when (response) {
+                        is UpdateCipherResponseJson.Invalid -> {
+                            IllegalStateException(response.message).asFailure()
+                        }
+
+                        is UpdateCipherResponseJson.Success -> {
+                            vaultDiskSource.saveCipher(userId = userId, cipher = response.cipher)
+                            response.cipher.toEncryptedSdkCipher().asSuccess()
+                        }
+                    }
+                }
+        } else {
+            vaultSdkSource.encryptCipher(userId = userId, cipherView = this)
+        }
 }

app/src/test/java/com/x8bit/bitwarden/data/vault/manager/CipherManagerTest.kt

@@ -34,6 +34,7 @@ import com.x8bit.bitwarden.data.vault.repository.model.DownloadAttachmentResult
 import com.x8bit.bitwarden.data.vault.repository.model.RestoreCipherResult
 import com.x8bit.bitwarden.data.vault.repository.model.ShareCipherResult
 import com.x8bit.bitwarden.data.vault.repository.model.UpdateCipherResult
+import com.x8bit.bitwarden.data.vault.repository.util.toEncryptedNetworkCipher
 import com.x8bit.bitwarden.data.vault.repository.util.toEncryptedNetworkCipherResponse
 import com.x8bit.bitwarden.data.vault.repository.util.toEncryptedSdkCipher
 import io.mockk.coEvery
@@ -69,7 +70,7 @@ class CipherManagerTest {
     private val vaultDiskSource: VaultDiskSource = mockk()
     private val vaultSdkSource: VaultSdkSource = mockk()
 
-    private val cipherManager: CipherManagerImpl = CipherManagerImpl(
+    private val cipherManager: CipherManager = CipherManagerImpl(
         ciphersService = ciphersService,
         vaultDiskSource = vaultDiskSource,
         vaultSdkSource = vaultSdkSource,
@@ -464,14 +465,20 @@ class CipherManagerTest {
     fun `softDeleteCipher with ciphersService softDeleteCipher failure should return DeleteCipherResult Error`() =
         runTest {
             fakeAuthDiskSource.userState = MOCK_USER_STATE
+            val userId = MOCK_USER_STATE.activeUserId
             val cipherId = "mockId-1"
+            val cipherView = createMockCipherView(number = 1)
+            val cipher = createMockSdkCipher(number = 1)
+            coEvery {
+                vaultSdkSource.encryptCipher(userId = userId, cipherView = cipherView)
+            } returns cipher.asSuccess()
             coEvery {
                 ciphersService.softDeleteCipher(cipherId = cipherId)
             } returns Throwable("Fail").asFailure()
 
             val result = cipherManager.softDeleteCipher(
                 cipherId = cipherId,
-                cipherView = createMockCipherView(number = 1),
+                cipherView = cipherView,
             )
 
             assertEquals(DeleteCipherResult.Error, result)
@@ -481,30 +488,31 @@ class CipherManagerTest {
     @Test
     fun `softDeleteCipher with ciphersService softDeleteCipher success should return DeleteCipherResult success`() =
         runTest {
-            mockkStatic(Cipher::toEncryptedNetworkCipherResponse)
-            every {
-                createMockSdkCipher(number = 1, clock = clock).toEncryptedNetworkCipherResponse()
-            } returns createMockCipher(number = 1)
             val fixedInstant = Instant.parse("2023-10-27T12:00:00Z")
             val userId = "mockId-1"
             val cipherId = "mockId-1"
+            val cipher = createMockSdkCipher(number = 1, clock = clock)
+            val cipherView = createMockCipherView(number = 1)
+            coEvery {
+                vaultSdkSource.encryptCipher(userId = userId, cipherView = cipherView)
+            } returns cipher.asSuccess()
             coEvery {
                 vaultSdkSource.encryptCipher(
                     userId = userId,
-                    cipherView = createMockCipherView(number = 1).copy(deletedDate = fixedInstant),
+                    cipherView = cipherView.copy(deletedDate = fixedInstant),
                 )
-            } returns createMockSdkCipher(number = 1, clock = clock).asSuccess()
+            } returns cipher.asSuccess()
+            coEvery {
+                vaultSdkSource.decryptCipher(userId = userId, cipher = cipher)
+            } returns cipherView.asSuccess()
             fakeAuthDiskSource.userState = MOCK_USER_STATE
             coEvery { ciphersService.softDeleteCipher(cipherId = cipherId) } returns Unit.asSuccess()
             coEvery {
                 vaultDiskSource.saveCipher(
                     userId = userId,
-                    cipher = createMockCipher(number = 1),
+                    cipher = cipher.toEncryptedNetworkCipherResponse(),
                 )
             } just runs
-            val cipherView = createMockCipherView(number = 1)
-            mockkStatic(Instant::class)
-            every { Instant.now() } returns fixedInstant
 
             val result = cipherManager.softDeleteCipher(
                 cipherId = cipherId,
@@ -512,8 +520,67 @@ class CipherManagerTest {
             )
 
             assertEquals(DeleteCipherResult.Success, result)
-            unmockkStatic(Instant::class)
+        }
-            unmockkStatic(Cipher::toEncryptedNetworkCipherResponse)
+
+    @Suppress("MaxLineLength")
+    @Test
+    fun `softDeleteCipher with cipher migration success should return DeleteCipherResult success`() =
+        runTest {
+            val fixedInstant = Instant.parse("2023-10-27T12:00:00Z")
+            val userId = "mockId-1"
+            val cipherId = "mockId-1"
+            val cipher = createMockSdkCipher(number = 1, clock = clock)
+            val cipherView = createMockCipherView(number = 1).copy(key = null)
+            val networkCipher = createMockCipher(number = 1).copy(key = null)
+            coEvery {
+                vaultSdkSource.encryptCipher(userId = userId, cipherView = cipherView)
+            } returns cipher.asSuccess()
+            coEvery {
+                ciphersService.updateCipher(
+                    cipherId = cipherId,
+                    body = cipher.toEncryptedNetworkCipher(),
+                )
+            } returns UpdateCipherResponseJson.Success(networkCipher).asSuccess()
+            coEvery {
+                vaultDiskSource.saveCipher(userId = userId, cipher = networkCipher)
+            } just runs
+            coEvery {
+                vaultSdkSource.decryptCipher(
+                    userId = userId,
+                    cipher = networkCipher.toEncryptedSdkCipher(),
+                )
+            } returns cipherView.asSuccess()
+            coEvery {
+                vaultSdkSource.encryptCipher(
+                    userId = userId,
+                    cipherView = cipherView.copy(deletedDate = fixedInstant),
+                )
+            } returns cipher.asSuccess()
+            coEvery {
+                vaultSdkSource.decryptCipher(userId = userId, cipher = cipher)
+            } returns cipherView.asSuccess()
+            fakeAuthDiskSource.userState = MOCK_USER_STATE
+            coEvery { ciphersService.softDeleteCipher(cipherId = cipherId) } returns Unit.asSuccess()
+            coEvery {
+                vaultDiskSource.saveCipher(
+                    userId = userId,
+                    cipher = cipher.toEncryptedNetworkCipherResponse(),
+                )
+            } just runs
+
+            val result = cipherManager.softDeleteCipher(
+                cipherId = cipherId,
+                cipherView = cipherView,
+            )
+
+            assertEquals(DeleteCipherResult.Success, result)
+            coVerify(exactly = 1) {
+                ciphersService.updateCipher(
+                    cipherId = cipherId,
+                    body = cipher.toEncryptedNetworkCipher(),
+                )
+                vaultDiskSource.saveCipher(userId = userId, cipher = networkCipher)
+            }
         }
 
     @Test