diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/CipherManagerImpl.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/CipherManagerImpl.kt index 8730c02e43..2f139c089f 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/CipherManagerImpl.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/vault/manager/CipherManagerImpl.kt @@ -7,6 +7,7 @@ import com.bitwarden.core.data.util.asSuccess import com.bitwarden.core.data.util.flatMap import com.bitwarden.network.model.AttachmentJsonResponse import com.bitwarden.network.model.CreateCipherInOrganizationJsonRequest +import com.bitwarden.network.model.CreateCipherResponseJson import com.bitwarden.network.model.ShareCipherJsonRequest import com.bitwarden.network.model.UpdateCipherCollectionsJsonRequest import com.bitwarden.network.model.UpdateCipherResponseJson @@ -52,19 +53,33 @@ class CipherManagerImpl( override suspend fun createCipher(cipherView: CipherView): CreateCipherResult { val userId = activeUserId - ?: return CreateCipherResult.Error(error = NoActiveUserException()) + ?: return CreateCipherResult.Error( + error = NoActiveUserException(), + errorMessage = null, + ) return vaultSdkSource .encryptCipher( userId = userId, cipherView = cipherView, ) .flatMap { ciphersService.createCipher(body = it.toEncryptedNetworkCipher()) } - .onSuccess { vaultDiskSource.saveCipher(userId = userId, cipher = it) } + .map { response -> + when (response) { + is CreateCipherResponseJson.Invalid -> { + CreateCipherResult.Error(errorMessage = response.message, error = null) + } + + is CreateCipherResponseJson.Success -> { + vaultDiskSource.saveCipher(userId = userId, cipher = response.cipher) + CreateCipherResult.Success + } + } + } .fold( - onFailure = { CreateCipherResult.Error(error = it) }, + onFailure = { CreateCipherResult.Error(errorMessage = null, error = it) }, onSuccess = { reviewPromptManager.registerAddCipherAction() - CreateCipherResult.Success + it }, ) } @@ -74,7 +89,7 @@ class CipherManagerImpl( collectionIds: List, ): CreateCipherResult { val userId = activeUserId - ?: return CreateCipherResult.Error(error = NoActiveUserException()) + ?: return CreateCipherResult.Error(errorMessage = null, error = NoActiveUserException()) return vaultSdkSource .encryptCipher( userId = userId, @@ -88,17 +103,26 @@ class CipherManagerImpl( ), ) } - .onSuccess { - vaultDiskSource.saveCipher( - userId = userId, - cipher = it.copy(collectionIds = collectionIds), - ) + .map { response -> + when (response) { + is CreateCipherResponseJson.Invalid -> { + CreateCipherResult.Error(errorMessage = response.message, error = null) + } + + is CreateCipherResponseJson.Success -> { + vaultDiskSource.saveCipher( + userId = userId, + cipher = response.cipher.copy(collectionIds = collectionIds), + ) + CreateCipherResult.Success + } + } } .fold( - onFailure = { CreateCipherResult.Error(error = it) }, + onFailure = { CreateCipherResult.Error(errorMessage = null, error = it) }, onSuccess = { reviewPromptManager.registerAddCipherAction() - CreateCipherResult.Success + it }, ) } diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/data/vault/repository/model/CreateCipherResult.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/vault/repository/model/CreateCipherResult.kt index 129aba101a..23d5487040 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/data/vault/repository/model/CreateCipherResult.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/vault/repository/model/CreateCipherResult.kt @@ -11,7 +11,8 @@ sealed class CreateCipherResult { data object Success : CreateCipherResult() /** - * Generic error while creating cipher. + * Generic error while creating cipher. The optional [errorMessage] may be displayed directly in + * the UI when present. */ - data class Error(val error: Throwable) : CreateCipherResult() + data class Error(val errorMessage: String?, val error: Throwable?) : CreateCipherResult() } diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt b/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt index e24a4c6dfb..a1c2d4d6b7 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt @@ -1598,7 +1598,11 @@ class VaultAddEditViewModel @Inject constructor( is CreateCipherResult.Error -> { showDialog( dialogState = VaultAddEditState.DialogState.Generic( - message = R.string.generic_error_message.asText(), + title = R.string.an_error_has_occurred.asText(), + message = result + .errorMessage + ?.asText() + ?: R.string.generic_error_message.asText(), error = result.error, ), ) diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/data/vault/manager/CipherManagerTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/data/vault/manager/CipherManagerTest.kt index 0e59933d90..a2ffdeecd6 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/data/vault/manager/CipherManagerTest.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/vault/manager/CipherManagerTest.kt @@ -6,6 +6,7 @@ import com.bitwarden.core.data.util.asFailure import com.bitwarden.core.data.util.asSuccess import com.bitwarden.network.model.AttachmentJsonRequest import com.bitwarden.network.model.CreateCipherInOrganizationJsonRequest +import com.bitwarden.network.model.CreateCipherResponseJson import com.bitwarden.network.model.ShareCipherJsonRequest import com.bitwarden.network.model.SyncResponseJson import com.bitwarden.network.model.UpdateCipherCollectionsJsonRequest @@ -46,6 +47,7 @@ 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 com.x8bit.bitwarden.data.vault.repository.util.toNetworkAttachmentRequest +import io.mockk.Ordering import io.mockk.coEvery import io.mockk.coVerify import io.mockk.every @@ -56,7 +58,6 @@ import io.mockk.mockkStatic import io.mockk.runs import io.mockk.unmockkConstructor import io.mockk.unmockkStatic -import io.mockk.verify import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.Assertions.assertEquals @@ -123,7 +124,13 @@ class CipherManagerTest { val result = cipherManager.createCipher(cipherView = mockk()) - assertEquals(CreateCipherResult.Error(error = NoActiveUserException()), result) + assertEquals( + CreateCipherResult.Error( + errorMessage = null, + error = NoActiveUserException(), + ), + result, + ) } @Test @@ -142,7 +149,7 @@ class CipherManagerTest { val result = cipherManager.createCipher(cipherView = mockCipherView) - assertEquals(CreateCipherResult.Error(error = error), result) + assertEquals(CreateCipherResult.Error(errorMessage = null, error = error), result) } @Test @@ -173,7 +180,46 @@ class CipherManagerTest { val result = cipherManager.createCipher(cipherView = mockCipherView) - assertEquals(CreateCipherResult.Error(error = error), result) + assertEquals(CreateCipherResult.Error(errorMessage = null, error = error), result) + } + + @Suppress("MaxLineLength") + @Test + fun `createCipher with ciphersService createCipher Invalid response should return CreateCipherResult failure`() = + runTest { + fakeAuthDiskSource.userState = MOCK_USER_STATE + val userId = "mockId-1" + val mockCipherView = createMockCipherView(number = 1) + coEvery { + vaultSdkSource.encryptCipher( + userId = userId, + cipherView = mockCipherView, + ) + } returns createMockEncryptionContext( + number = 1, + cipher = createMockSdkCipher(number = 1, clock = clock), + ).asSuccess() + coEvery { + ciphersService.createCipher( + body = createMockCipherJsonRequest( + number = 1, + login = createMockLogin(number = 1, uri = null), + ), + ) + } returns CreateCipherResponseJson.Invalid( + message = "You do not have permission to edit this.", + validationErrors = null, + ).asSuccess() + + val result = cipherManager.createCipher(cipherView = mockCipherView) + + assertEquals( + CreateCipherResult.Error( + errorMessage = "You do not have permission to edit this.", + error = null, + ), + result, + ) } @Test @@ -200,13 +246,16 @@ class CipherManagerTest { login = createMockLogin(number = 1, uri = null), ), ) - } returns mockCipher.asSuccess() + } returns CreateCipherResponseJson.Success(mockCipher).asSuccess() coEvery { vaultDiskSource.saveCipher(userId, mockCipher) } just runs val result = cipherManager.createCipher(cipherView = mockCipherView) assertEquals(CreateCipherResult.Success, result) - verify(exactly = 1) { reviewPromptManager.registerAddCipherAction() } + coVerify(ordering = Ordering.ORDERED) { + vaultDiskSource.saveCipher(userId, mockCipher) + reviewPromptManager.registerAddCipherAction() + } } @Test @@ -219,7 +268,13 @@ class CipherManagerTest { collectionIds = mockk(), ) - assertEquals(CreateCipherResult.Error(error = NoActiveUserException()), result) + assertEquals( + CreateCipherResult.Error( + errorMessage = null, + error = NoActiveUserException(), + ), + result, + ) } @Test @@ -242,7 +297,7 @@ class CipherManagerTest { collectionIds = mockk(), ) - assertEquals(CreateCipherResult.Error(error = error), result) + assertEquals(CreateCipherResult.Error(errorMessage = null, error = error), result) } @Test @@ -279,7 +334,52 @@ class CipherManagerTest { collectionIds = listOf("mockId-1"), ) - assertEquals(CreateCipherResult.Error(error = error), result) + assertEquals(CreateCipherResult.Error(errorMessage = null, error = error), result) + } + + @Suppress("MaxLineLength") + @Test + fun `createCipherInOrganization with ciphersService createCipher Invalid response should return CreateCipherResult failure`() = + runTest { + fakeAuthDiskSource.userState = MOCK_USER_STATE + val userId = "mockId-1" + val mockCipherView = createMockCipherView(number = 1) + coEvery { + vaultSdkSource.encryptCipher( + userId = userId, + cipherView = mockCipherView, + ) + } returns createMockEncryptionContext( + number = 1, + cipher = createMockSdkCipher(number = 1, clock = clock), + ).asSuccess() + coEvery { + ciphersService.createCipherInOrganization( + body = CreateCipherInOrganizationJsonRequest( + cipher = createMockCipherJsonRequest( + number = 1, + login = createMockLogin(number = 1, uri = null), + ), + collectionIds = listOf("mockId-1"), + ), + ) + } returns CreateCipherResponseJson.Invalid( + message = "You do not have permission to edit this.", + validationErrors = null, + ).asSuccess() + + val result = cipherManager.createCipherInOrganization( + cipherView = mockCipherView, + collectionIds = listOf("mockId-1"), + ) + + assertEquals( + CreateCipherResult.Error( + errorMessage = "You do not have permission to edit this.", + error = null, + ), + result, + ) } @Test @@ -309,7 +409,7 @@ class CipherManagerTest { collectionIds = listOf("mockId-1"), ), ) - } returns mockCipher.asSuccess() + } returns CreateCipherResponseJson.Success(mockCipher).asSuccess() coEvery { vaultDiskSource.saveCipher( userId, @@ -323,7 +423,13 @@ class CipherManagerTest { ) assertEquals(CreateCipherResult.Success, result) - verify(exactly = 1) { reviewPromptManager.registerAddCipherAction() } + coVerify(ordering = Ordering.ORDERED) { + vaultDiskSource.saveCipher( + userId, + mockCipher.copy(collectionIds = listOf("mockId-1")), + ) + reviewPromptManager.registerAddCipherAction() + } } @Test diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt index 01206c13fe..c8215e89b8 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt @@ -1876,7 +1876,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { coEvery { vaultRepository.createCipherInOrganization(any(), any()) - } returns CreateCipherResult.Error(error = Throwable("Oh dang")) + } returns CreateCipherResult.Error(errorMessage = null, error = Throwable("Oh dang")) viewModel.trySendAction(VaultAddEditAction.Common.SaveClick) assertEquals( @@ -1890,6 +1890,61 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { ) } + @Test + fun `in edit mode, SaveClick should show network error message in dialog when not null`() = + runTest { + val stateWithName = createVaultAddItemState( + commonContentViewState = createCommonContentViewState( + name = "mockName-1", + ), + ) + mutableVaultDataFlow.value = DataState.Loaded(createVaultData()) + + val viewModel = createAddVaultItemViewModel( + createSavedStateHandleWithState( + state = stateWithName, + vaultAddEditType = VaultAddEditType.AddItem, + vaultItemCipherType = VaultItemCipherType.LOGIN, + ), + ) + + coEvery { + vaultRepository.createCipherInOrganization(any(), any()) + } returns CreateCipherResult.Error( + errorMessage = "Network error message", + error = null, + ) + viewModel.trySendAction(VaultAddEditAction.Common.SaveClick) + + assertEquals( + stateWithName.copy( + dialog = VaultAddEditState.DialogState.Generic( + title = R.string.an_error_has_occurred.asText(), + message = "Network error message".asText(), + ), + ), + viewModel.stateFlow.value, + ) + + // Verify default error message is shown when errorMessage is null + coEvery { + vaultRepository.createCipherInOrganization(any(), any()) + } returns CreateCipherResult.Error( + errorMessage = null, + error = null, + ) + viewModel.trySendAction(VaultAddEditAction.Common.SaveClick) + assertEquals( + stateWithName.copy( + dialog = VaultAddEditState.DialogState.Generic( + title = R.string.an_error_has_occurred.asText(), + message = R.string.generic_error_message.asText(), + ), + ), + viewModel.stateFlow.value, + ) + } + @Test fun `in edit mode, SaveClick should show dialog, and remove it once an item is saved`() = runTest { diff --git a/network/src/main/kotlin/com/bitwarden/network/model/CreateCipherResponseJson.kt b/network/src/main/kotlin/com/bitwarden/network/model/CreateCipherResponseJson.kt new file mode 100644 index 0000000000..841164ef22 --- /dev/null +++ b/network/src/main/kotlin/com/bitwarden/network/model/CreateCipherResponseJson.kt @@ -0,0 +1,32 @@ +package com.bitwarden.network.model + +import kotlinx.serialization.SerialName +import kotlinx.serialization.Serializable + +/** + * Models the response from the create cipher request. + */ +sealed class CreateCipherResponseJson { + + /** + * The request completed successfully and returned the created [cipher]. + */ + data class Success(val cipher: SyncResponseJson.Cipher) : CreateCipherResponseJson() + + /** + * Represents the json body of an invalid create request. + * + * @param message A general, user-displayable error message. + * @param validationErrors a map where each value is a list of error messages for each key. + * The values in the array should be used for display to the user, since the keys tend to come + * back as nonsense. (eg: empty string key) + */ + @Serializable + data class Invalid( + @SerialName("message") + val message: String?, + + @SerialName("validationErrors") + val validationErrors: Map>?, + ) : CreateCipherResponseJson() +} diff --git a/network/src/main/kotlin/com/bitwarden/network/service/CiphersService.kt b/network/src/main/kotlin/com/bitwarden/network/service/CiphersService.kt index 908dabc4f8..fb93efe89b 100644 --- a/network/src/main/kotlin/com/bitwarden/network/service/CiphersService.kt +++ b/network/src/main/kotlin/com/bitwarden/network/service/CiphersService.kt @@ -5,6 +5,7 @@ import com.bitwarden.network.model.AttachmentJsonRequest import com.bitwarden.network.model.AttachmentJsonResponse import com.bitwarden.network.model.CipherJsonRequest import com.bitwarden.network.model.CreateCipherInOrganizationJsonRequest +import com.bitwarden.network.model.CreateCipherResponseJson import com.bitwarden.network.model.ImportCiphersJsonRequest import com.bitwarden.network.model.ImportCiphersResponseJson import com.bitwarden.network.model.ShareCipherJsonRequest @@ -21,14 +22,14 @@ interface CiphersService { /** * Attempt to create a cipher. */ - suspend fun createCipher(body: CipherJsonRequest): Result + suspend fun createCipher(body: CipherJsonRequest): Result /** * Attempt to create a cipher that belongs to an organization. */ suspend fun createCipherInOrganization( body: CreateCipherInOrganizationJsonRequest, - ): Result + ): Result /** * Attempt to upload an attachment file. diff --git a/network/src/main/kotlin/com/bitwarden/network/service/CiphersServiceImpl.kt b/network/src/main/kotlin/com/bitwarden/network/service/CiphersServiceImpl.kt index 32f1f65956..371d4b2d4f 100644 --- a/network/src/main/kotlin/com/bitwarden/network/service/CiphersServiceImpl.kt +++ b/network/src/main/kotlin/com/bitwarden/network/service/CiphersServiceImpl.kt @@ -8,6 +8,7 @@ import com.bitwarden.network.model.AttachmentJsonRequest import com.bitwarden.network.model.AttachmentJsonResponse import com.bitwarden.network.model.CipherJsonRequest import com.bitwarden.network.model.CreateCipherInOrganizationJsonRequest +import com.bitwarden.network.model.CreateCipherResponseJson import com.bitwarden.network.model.FileUploadType import com.bitwarden.network.model.ImportCiphersJsonRequest import com.bitwarden.network.model.ImportCiphersResponseJson @@ -36,16 +37,38 @@ internal class CiphersServiceImpl( private val json: Json, private val clock: Clock, ) : CiphersService { - override suspend fun createCipher(body: CipherJsonRequest): Result = + override suspend fun createCipher( + body: CipherJsonRequest, + ): Result = ciphersApi .createCipher(body = body) .toResult() + .map { CreateCipherResponseJson.Success(it) } + .recoverCatching { throwable -> + throwable + .toBitwardenError() + .parseErrorBodyOrNull( + code = NetworkErrorCode.BAD_REQUEST, + json = json, + ) + ?: throw throwable + } override suspend fun createCipherInOrganization( body: CreateCipherInOrganizationJsonRequest, - ): Result = ciphersApi + ): Result = ciphersApi .createCipherInOrganization(body = body) .toResult() + .map { CreateCipherResponseJson.Success(it) } + .recoverCatching { throwable -> + throwable + .toBitwardenError() + .parseErrorBodyOrNull( + code = NetworkErrorCode.BAD_REQUEST, + json = json, + ) + ?: throw throwable + } override suspend fun createAttachment( cipherId: String, diff --git a/network/src/test/kotlin/com/bitwarden/network/service/CiphersServiceTest.kt b/network/src/test/kotlin/com/bitwarden/network/service/CiphersServiceTest.kt index bba7f1d8cd..3abe6f2ada 100644 --- a/network/src/test/kotlin/com/bitwarden/network/service/CiphersServiceTest.kt +++ b/network/src/test/kotlin/com/bitwarden/network/service/CiphersServiceTest.kt @@ -6,6 +6,7 @@ import com.bitwarden.network.api.CiphersApi import com.bitwarden.network.base.BaseServiceTest import com.bitwarden.network.model.AttachmentJsonResponse import com.bitwarden.network.model.CreateCipherInOrganizationJsonRequest +import com.bitwarden.network.model.CreateCipherResponseJson import com.bitwarden.network.model.FileUploadType import com.bitwarden.network.model.ImportCiphersJsonRequest import com.bitwarden.network.model.ImportCiphersResponseJson @@ -67,7 +68,22 @@ class CiphersServiceTest : BaseServiceTest() { body = createMockCipherJsonRequest(number = 1), ) assertEquals( - createMockCipher(number = 1), + CreateCipherResponseJson.Success(createMockCipher(number = 1)), + result.getOrThrow(), + ) + } + + @Test + fun `createCipher should return Invalid with correct data`() = runTest { + server.enqueue(MockResponse().setResponseCode(400).setBody(CREATE_CIPHER_INVALID_JSON)) + val result = ciphersService.createCipher( + body = createMockCipherJsonRequest(number = 1), + ) + assertEquals( + CreateCipherResponseJson.Invalid( + message = "Cipher was not encrypted for the current user. Please try again.", + validationErrors = null, + ), result.getOrThrow(), ) } @@ -82,11 +98,30 @@ class CiphersServiceTest : BaseServiceTest() { ), ) assertEquals( - createMockCipher(number = 1), + CreateCipherResponseJson.Success(createMockCipher(number = 1)), result.getOrThrow(), ) } + @Test + fun `createCipherInOrganization should return Invalid with correct data`() = + runTest { + server.enqueue(MockResponse().setResponseCode(400).setBody(CREATE_CIPHER_INVALID_JSON)) + val result = ciphersService.createCipherInOrganization( + body = CreateCipherInOrganizationJsonRequest( + cipher = createMockCipherJsonRequest(number = 1), + collectionIds = listOf("12345"), + ), + ) + assertEquals( + CreateCipherResponseJson.Invalid( + message = "Cipher was not encrypted for the current user. Please try again.", + validationErrors = null, + ), + result.getOrThrow(), + ) + } + @Test fun `createAttachment should return the correct response`() = runTest { server.enqueue(MockResponse().setBody(CREATE_ATTACHMENT_SUCCESS_JSON)) @@ -606,6 +641,12 @@ private const val UPDATE_CIPHER_INVALID_JSON = """ "validationErrors": null } """ +private const val CREATE_CIPHER_INVALID_JSON = """ +{ + "message": "Cipher was not encrypted for the current user. Please try again.", + "validationErrors": null +} +""" private const val GET_CIPHER_ATTACHMENT_SUCCESS_JSON = """ {