PM-19334: Propagate errors to the UI (#4893)

This commit is contained in:
David Perez authored and GitHub committed 2025-03-18 17:27:41 +00:00
1 parent 475a82e0fb
commit f4f669683e
18 files changed
+136 -115

No files matched your search

@@ -5,6 +5,7 @@ import android.security.KeyChain
import android.security.KeyChainException
import com.x8bit.bitwarden.data.platform.datasource.disk.model.MutualTlsCertificate
import com.x8bit.bitwarden.data.platform.datasource.disk.model.MutualTlsKeyHost
import com.x8bit.bitwarden.data.platform.error.MissingPropertyException
import com.x8bit.bitwarden.data.platform.manager.model.ImportPrivateKeyResult
import timber.log.Timber
import java.io.IOException
@@ -24,7 +25,7 @@ class KeyManagerImpl(
private val context: Context,
) : KeyManager {
@Suppress("CyclomaticComplexMethod")
@Suppress("LongMethod", "CyclomaticComplexMethod")
override fun importMutualTlsCertificate(
key: ByteArray,
alias: String,
@@ -35,28 +36,29 @@ class KeyManagerImpl(
.inputStream()
.use { stream ->
try {
KeyStore.getInstance(KEYSTORE_TYPE_PKCS12)
KeyStore
.getInstance(KEYSTORE_TYPE_PKCS12)
.also { it.load(stream, password.toCharArray()) }
} catch (e: KeyStoreException) {
Timber.Forest.e(e, "Failed to load PKCS12 bytes")
return ImportPrivateKeyResult.Error.UnsupportedKey
return ImportPrivateKeyResult.Error.UnsupportedKey(throwable = e)
} catch (e: IOException) {
Timber.Forest.e(e, "Format or password error while loading PKCS12 bytes")
return when (e.cause) {
is UnrecoverableKeyException -> {
ImportPrivateKeyResult.Error.UnrecoverableKey
ImportPrivateKeyResult.Error.UnrecoverableKey(throwable = e)
}
else -> {
ImportPrivateKeyResult.Error.KeyStoreOperationFailed
ImportPrivateKeyResult.Error.KeyStoreOperationFailed(throwable = e)
}
}
} catch (e: CertificateException) {
Timber.Forest.e(e, "Unable to load certificate chain")
return ImportPrivateKeyResult.Error.InvalidCertificateChain
return ImportPrivateKeyResult.Error.InvalidCertificateChain(throwable = e)
} catch (e: NoSuchAlgorithmException) {
Timber.Forest.e(e, "Cryptographic algorithm not supported")
return ImportPrivateKeyResult.Error.UnsupportedKey
return ImportPrivateKeyResult.Error.UnsupportedKey(throwable = e)
}
}
@@ -64,22 +66,29 @@ class KeyManagerImpl(
val internalAlias = pkcs12KeyStore.aliases()
?.takeIf { it.hasMoreElements() }
?.nextElement()
?: return ImportPrivateKeyResult.Error.UnsupportedKey
?: return ImportPrivateKeyResult.Error.UnsupportedKey(
throwable = MissingPropertyException("Internal Alias"),
)
// Step 3: Extract PrivateKey and X.509 certificate from the KeyStore and verify
// certificate alias.
val privateKey = try {
pkcs12KeyStore.getKey(internalAlias, password.toCharArray())
?: return ImportPrivateKeyResult.Error.UnrecoverableKey
pkcs12KeyStore
.getKey(internalAlias, password.toCharArray())
?: return ImportPrivateKeyResult.Error.UnrecoverableKey(
throwable = MissingPropertyException("Private Key"),
)
} catch (e: UnrecoverableKeyException) {
Timber.Forest.e(e, "Failed to get private key")
return ImportPrivateKeyResult.Error.UnrecoverableKey
return ImportPrivateKeyResult.Error.UnrecoverableKey(throwable = e)
}
val certChain: Array<Certificate> = pkcs12KeyStore
.getCertificateChain(internalAlias)
?.takeUnless { it.isEmpty() }
?: return ImportPrivateKeyResult.Error.InvalidCertificateChain
?: return ImportPrivateKeyResult.Error.InvalidCertificateChain(
throwable = MissingPropertyException("Certificate Chain"),
)
// Step 4: Store the private key and X.509 certificate in the AndroidKeyStore if the alias
// does not exists.
@@ -92,7 +101,7 @@ class KeyManagerImpl(
setKeyEntry(alias, privateKey, null, certChain)
} catch (e: KeyStoreException) {
Timber.Forest.e(e, "Failed to import key into Android KeyStore")
return ImportPrivateKeyResult.Error.KeyStoreOperationFailed
return ImportPrivateKeyResult.Error.KeyStoreOperationFailed(throwable = e)
}
}
return ImportPrivateKeyResult.Success(alias)
@@ -16,30 +16,44 @@ sealed class ImportPrivateKeyResult {
* Represents a generic error during the import process.
*/
sealed class Error : ImportPrivateKeyResult() {
/**
* The underlying error.
*/
abstract val throwable: Throwable?
/**
* Indicates that the provided key is unrecoverable or the password is incorrect.
*/
data object UnrecoverableKey : Error()
data class UnrecoverableKey(
override val throwable: Throwable,
) : Error()
/**
* Indicates that the certificate chain associated with the key is invalid.
*/
data object InvalidCertificateChain : Error()
data class InvalidCertificateChain(
override val throwable: Throwable,
) : Error()
/**
* Indicates that the specified alias is already in use.
*/
data object DuplicateAlias : Error()
data object DuplicateAlias : Error() {
override val throwable: Throwable? = null
}
/**
* Indicates that an error occurred during the key store operation.
*/
data object KeyStoreOperationFailed : Error()
data class KeyStoreOperationFailed(
override val throwable: Throwable,
) : Error()
/**
* Indicates the provided key is not supported.
*/
data object UnsupportedKey : Error()
data class UnsupportedKey(
override val throwable: Throwable,
) : Error()
}
}
@@ -494,7 +494,8 @@ class SettingsRepositoryImpl(
}
override suspend fun setupBiometricsKey(cipher: Cipher): BiometricsKeyResult {
val userId = activeUserId ?: return BiometricsKeyResult.Error
val userId = activeUserId
?: return BiometricsKeyResult.Error(error = NoActiveUserException())
return vaultSdkSource
.getUserEncryptionKey(userId = userId)
.onSuccess { biometricsKey ->
@@ -508,7 +509,7 @@ class SettingsRepositoryImpl(
}
.fold(
onSuccess = { BiometricsKeyResult.Success },
onFailure = { BiometricsKeyResult.Error },
onFailure = { BiometricsKeyResult.Error(error = it) },
)
}
@@ -12,5 +12,7 @@ sealed class BiometricsKeyResult {
/**
* Generic error while setting up the biometrics key.
*/
data object Error : BiometricsKeyResult()
data class Error(
val error: Throwable,
) : BiometricsKeyResult()
}
@@ -1,19 +0,0 @@
package com.x8bit.bitwarden.data.vault.datasource.sdk.model
import com.bitwarden.vault.CipherView
/**
* Models the result of querying for ciphers with FIDO 2 credentials.
*/
sealed class FindFido2CredentialsResult {
/**
* Indicates the query was executed successfully.
*/
data class Success(val cipherViews: List<CipherView>) : FindFido2CredentialsResult()
/**
* Indicates the query was not executed successfully.
*/
data object Error : FindFido2CredentialsResult()
}
@@ -1,17 +0,0 @@
package com.x8bit.bitwarden.data.vault.datasource.sdk.model
/**
* Models the result of saving a FIDO 2 credential.
*/
sealed class SaveCredentialResult {
/**
* Indicates the credential has been saved.
*/
data object Success : SaveCredentialResult()
/**
* Indicates the credential was not saved.
*/
data object Error : SaveCredentialResult()
}
@@ -446,7 +446,10 @@ class CipherManagerImpl(
?: return IllegalStateException("Attachment does not have a url").asFailure()
val encryptedFile = when (val result = fileManager.downloadFileToCache(url)) {
DownloadResult.Failure -> return IllegalStateException("Download failed").asFailure()
is DownloadResult.Failure -> {
return IllegalStateException("Download failed", result.error).asFailure()
}
is DownloadResult.Success -> result.file
}
@@ -44,7 +44,7 @@ class FileManagerImpl(
.getDataStream(url)
.fold(
onSuccess = { it },
onFailure = { return DownloadResult.Failure },
onFailure = { return DownloadResult.Failure(error = it) },
)
// Create a temporary file in cache to write to
@@ -66,7 +66,7 @@ class FileManagerImpl(
}
fos.flush()
} catch (e: RuntimeException) {
return@withContext DownloadResult.Failure
return@withContext DownloadResult.Failure(error = e)
}
}
}
@@ -94,7 +94,7 @@ class FileManagerImpl(
}
}
true
} catch (exception: RuntimeException) {
} catch (_: RuntimeException) {
false
}
}
@@ -111,7 +111,7 @@ class FileManagerImpl(
}
}
true
} catch (exception: RuntimeException) {
} catch (_: RuntimeException) {
false
}
}
@@ -14,5 +14,7 @@ sealed class DownloadResult {
/**
* The download failed.
*/
data object Failure : DownloadResult()
data class Failure(
val error: Throwable,
) : DownloadResult()
}
@@ -164,7 +164,7 @@ class SetupUnlockViewModel @Inject constructor(
settingsRepository.storeUnlockPin(
pin = state.pin,
shouldRequireMasterPasswordOnRestart =
state.shouldRequireMasterPasswordOnRestart,
state.shouldRequireMasterPasswordOnRestart,
)
}
}
@@ -182,7 +182,7 @@ class SetupUnlockViewModel @Inject constructor(
action: SetupUnlockAction.Internal.BiometricsKeyResultReceive,
) {
when (action.result) {
BiometricsKeyResult.Error -> {
is BiometricsKeyResult.Error -> {
mutableStateFlow.update {
it.copy(
dialogState = null,
@@ -74,8 +74,8 @@ class AccountSecurityViewModel @Inject constructor(
?.hasMasterPassword != false,
isUnlockWithPinEnabled = settingsRepository.isUnlockWithPinEnabled,
shouldShowEnableAuthenticatorSync =
featureFlagManager.getFeatureFlag(FlagKey.AuthenticatorSync) &&
!isBuildVersionBelow(Build.VERSION_CODES.S),
featureFlagManager.getFeatureFlag(FlagKey.AuthenticatorSync) &&
!isBuildVersionBelow(Build.VERSION_CODES.S),
userId = userId,
vaultTimeout = settingsRepository.vaultTimeout,
vaultTimeoutAction = settingsRepository.vaultTimeoutAction,
@@ -365,7 +365,7 @@ class AccountSecurityViewModel @Inject constructor(
settingsRepository.storeUnlockPin(
pin = state.pin,
shouldRequireMasterPasswordOnRestart =
state.shouldRequireMasterPasswordOnRestart,
state.shouldRequireMasterPasswordOnRestart,
)
}
}
@@ -464,7 +464,7 @@ class AccountSecurityViewModel @Inject constructor(
action: AccountSecurityAction.Internal.BiometricsKeyResultReceive,
) {
when (action.result) {
BiometricsKeyResult.Error -> {
is BiometricsKeyResult.Error -> {
mutableStateFlow.update {
it.copy(
dialog = null,
@@ -316,7 +316,7 @@ class AuthRepositoryTest {
GetTokenResponseJson.Success::toUserState,
UserStateJson::toRemovedPasswordUserStateJson,
)
unmockkStatic(
mockkConstructor(
NoActiveUserException::class,
MissingPropertyException::class,
)
@@ -3,14 +3,17 @@ package com.x8bit.bitwarden.data.platform.manager
import android.content.Context
import android.security.KeyChain
import android.security.KeyChainException
import com.x8bit.bitwarden.data.platform.manager.model.ImportPrivateKeyResult
import com.x8bit.bitwarden.data.platform.datasource.disk.model.MutualTlsCertificate
import com.x8bit.bitwarden.data.platform.datasource.disk.model.MutualTlsKeyHost
import com.x8bit.bitwarden.data.platform.error.MissingPropertyException
import com.x8bit.bitwarden.data.platform.manager.model.ImportPrivateKeyResult
import io.mockk.every
import io.mockk.just
import io.mockk.mockk
import io.mockk.mockkConstructor
import io.mockk.mockkStatic
import io.mockk.runs
import io.mockk.unmockkConstructor
import io.mockk.unmockkStatic
import io.mockk.verify
import org.junit.jupiter.api.AfterEach
@@ -39,11 +42,16 @@ class KeyManagerTest {
@BeforeEach
fun setUp() {
mockkStatic(KeyStore::class, KeyChain::class)
mockkConstructor(MissingPropertyException::class)
every {
anyConstructed<MissingPropertyException>() == any<MissingPropertyException>()
} returns true
}
@AfterEach
fun tearDown() {
unmockkStatic(KeyStore::class, KeyChain::class)
unmockkConstructor(MissingPropertyException::class)
}
@Test
@@ -367,11 +375,10 @@ class KeyManagerTest {
val password = "password"
// Verify KeyStoreException is handled
every {
mockPkcs12KeyStore.load(any(), any())
} throws KeyStoreException()
val keystoreError = KeyStoreException()
every { mockPkcs12KeyStore.load(any(), any()) } throws keystoreError
assertEquals(
ImportPrivateKeyResult.Error.UnsupportedKey,
ImportPrivateKeyResult.Error.UnsupportedKey(throwable = keystoreError),
keyDiskSource.importMutualTlsCertificate(
key = pkcs12Bytes,
alias = expectedAlias,
@@ -380,11 +387,12 @@ class KeyManagerTest {
) { "KeyStoreException was not handled correctly" }
// Verify IOException is handled
every {
mockPkcs12KeyStore.load(any(), any())
} throws IOException()
val keystoreOperationIoError = IOException()
every { mockPkcs12KeyStore.load(any(), any()) } throws keystoreOperationIoError
assertEquals(
ImportPrivateKeyResult.Error.KeyStoreOperationFailed,
ImportPrivateKeyResult.Error.KeyStoreOperationFailed(
throwable = keystoreOperationIoError,
),
keyDiskSource.importMutualTlsCertificate(
key = pkcs12Bytes,
alias = expectedAlias,
@@ -393,12 +401,10 @@ class KeyManagerTest {
) { "IOException was not handled correctly" }
// Verify IOException with UnrecoverableKeyException cause is handled
every {
mockPkcs12KeyStore.load(any(), any())
} throws IOException(UnrecoverableKeyException())
val unrecoverableKeyError = IOException(UnrecoverableKeyException())
every { mockPkcs12KeyStore.load(any(), any()) } throws unrecoverableKeyError
assertEquals(
ImportPrivateKeyResult.Error.UnrecoverableKey,
ImportPrivateKeyResult.Error.UnrecoverableKey(throwable = unrecoverableKeyError),
keyDiskSource.importMutualTlsCertificate(
key = pkcs12Bytes,
alias = expectedAlias,
@@ -407,11 +413,12 @@ class KeyManagerTest {
)
// Verify IOException with unexpected cause is handled
every {
mockPkcs12KeyStore.load(any(), any())
} throws IOException(Exception())
val keystoreOperationError = IOException(Exception())
every { mockPkcs12KeyStore.load(any(), any()) } throws keystoreOperationError
assertEquals(
ImportPrivateKeyResult.Error.KeyStoreOperationFailed,
ImportPrivateKeyResult.Error.KeyStoreOperationFailed(
throwable = keystoreOperationError,
),
keyDiskSource.importMutualTlsCertificate(
key = pkcs12Bytes,
alias = expectedAlias,
@@ -420,11 +427,10 @@ class KeyManagerTest {
) { "IOException with Unexpected exception cause was not handled correctly" }
// Verify CertificateException is handled
every {
mockPkcs12KeyStore.load(any(), any())
} throws CertificateException()
val certificateError = CertificateException()
every { mockPkcs12KeyStore.load(any(), any()) } throws certificateError
assertEquals(
ImportPrivateKeyResult.Error.InvalidCertificateChain,
ImportPrivateKeyResult.Error.InvalidCertificateChain(throwable = certificateError),
keyDiskSource.importMutualTlsCertificate(
key = pkcs12Bytes,
alias = expectedAlias,
@@ -433,11 +439,10 @@ class KeyManagerTest {
) { "CertificateException was not handled correctly" }
// Verify NoSuchAlgorithmException is handled
every {
mockPkcs12KeyStore.load(any(), any())
} throws NoSuchAlgorithmException()
val algorithmError = NoSuchAlgorithmException()
every { mockPkcs12KeyStore.load(any(), any()) } throws algorithmError
assertEquals(
ImportPrivateKeyResult.Error.UnsupportedKey,
ImportPrivateKeyResult.Error.UnsupportedKey(throwable = algorithmError),
keyDiskSource.importMutualTlsCertificate(
key = pkcs12Bytes,
alias = expectedAlias,
@@ -458,7 +463,9 @@ class KeyManagerTest {
}
assertEquals(
ImportPrivateKeyResult.Error.UnsupportedKey,
ImportPrivateKeyResult.Error.UnsupportedKey(
throwable = MissingPropertyException("Internal Alias"),
),
keyDiskSource.importMutualTlsCertificate(
key = pkcs12Bytes,
alias = expectedAlias,
@@ -481,15 +488,16 @@ class KeyManagerTest {
every { hasMoreElements() } returns true
every { nextElement() } returns "mockInternalAlias"
}
val error = UnrecoverableKeyException()
every {
mockPkcs12KeyStore.getKey(
"mockInternalAlias",
password.toCharArray(),
)
} throws UnrecoverableKeyException()
} throws error
assertEquals(
ImportPrivateKeyResult.Error.UnrecoverableKey,
ImportPrivateKeyResult.Error.UnrecoverableKey(throwable = error),
keyDiskSource.importMutualTlsCertificate(
key = pkcs12Bytes,
alias = expectedAlias,
@@ -504,7 +512,9 @@ class KeyManagerTest {
)
} returns null
assertEquals(
ImportPrivateKeyResult.Error.UnrecoverableKey,
ImportPrivateKeyResult.Error.UnrecoverableKey(
throwable = MissingPropertyException("Private Key"),
),
keyDiskSource.importMutualTlsCertificate(
key = pkcs12Bytes,
alias = expectedAlias,
@@ -537,7 +547,9 @@ class KeyManagerTest {
mockPkcs12KeyStore.getCertificateChain("mockInternalAlias")
} returns emptyArray()
assertEquals(
ImportPrivateKeyResult.Error.InvalidCertificateChain,
ImportPrivateKeyResult.Error.InvalidCertificateChain(
throwable = MissingPropertyException("Certificate Chain"),
),
keyDiskSource.importMutualTlsCertificate(
key = pkcs12Bytes,
alias = expectedAlias,
@@ -550,7 +562,9 @@ class KeyManagerTest {
mockPkcs12KeyStore.getCertificateChain("mockInternalAlias")
} returns null
assertEquals(
ImportPrivateKeyResult.Error.InvalidCertificateChain,
ImportPrivateKeyResult.Error.InvalidCertificateChain(
throwable = MissingPropertyException("Certificate Chain"),
),
keyDiskSource.importMutualTlsCertificate(
key = pkcs12Bytes,
alias = expectedAlias,
@@ -563,9 +577,11 @@ class KeyManagerTest {
@Test
fun `importMutualTlsCertificate should return KeyStoreOperationFailed when saving to Android KeyStore throws KeyStoreException`() {
setupMockAndroidKeyStore()
setupMockPkcs12KeyStore()
val expectedAlias = "mockAlias"
val pkcs12Bytes = "key.p12".toByteArray()
val password = "password"
val error = KeyStoreException()
every { mockPkcs12KeyStore.aliases() } returns mockk {
every { hasMoreElements() } returns true
@@ -582,6 +598,8 @@ class KeyManagerTest {
mockPkcs12KeyStore.getCertificateChain("mockInternalAlias")
} returns arrayOf(mockk())
every { mockAndroidKeyStore.containsAlias(expectedAlias) } returns false
every {
mockAndroidKeyStore.setKeyEntry(
expectedAlias,
@@ -589,10 +607,10 @@ class KeyManagerTest {
any(),
any(),
)
} throws KeyStoreException()
} throws error
assertEquals(
ImportPrivateKeyResult.Error.KeyStoreOperationFailed,
ImportPrivateKeyResult.Error.KeyStoreOperationFailed(throwable = error),
keyDiskSource.importMutualTlsCertificate(
key = pkcs12Bytes,
alias = expectedAlias,
@@ -833,7 +833,7 @@ class SettingsRepositoryTest {
val result = settingsRepository.setupBiometricsKey(cipher = cipher)
assertEquals(BiometricsKeyResult.Error, result)
assertEquals(BiometricsKeyResult.Error(error = NoActiveUserException()), result)
coVerify(exactly = 0) {
vaultSdkSource.getUserEncryptionKey(userId = any())
}
@@ -845,13 +845,14 @@ class SettingsRepositoryTest {
runTest {
fakeAuthDiskSource.userState = MOCK_USER_STATE
val cipher = mockk<Cipher>()
val error = Throwable("Fail")
coEvery {
vaultSdkSource.getUserEncryptionKey(userId = USER_ID)
} returns Throwable("Fail").asFailure()
} returns error.asFailure()
val result = settingsRepository.setupBiometricsKey(cipher = cipher)
assertEquals(BiometricsKeyResult.Error, result)
assertEquals(BiometricsKeyResult.Error(error = error), result)
coVerify(exactly = 1) {
vaultSdkSource.getUserEncryptionKey(userId = USER_ID)
}
@@ -1857,10 +1857,9 @@ class CipherManagerTest {
coEvery {
ciphersService.getCipherAttachment(cipherId = any(), attachmentId = any())
} returns response.asSuccess()
coEvery {
fileManager.downloadFileToCache(url = any())
} returns DownloadResult.Failure
} returns DownloadResult.Failure(error = Throwable("Fail!"))
assertEquals(
DownloadAttachmentResult.Failure(IllegalStateException()),
@@ -169,7 +169,7 @@ class SetupUnlockViewModelTest : BaseViewModelTest() {
runTest {
coEvery {
settingsRepository.setupBiometricsKey(CIPHER)
} returns BiometricsKeyResult.Error
} returns BiometricsKeyResult.Error(error = Throwable("Fail!"))
val viewModel = createViewModel()
viewModel.stateFlow.test {
@@ -519,7 +519,9 @@ class EnvironmentViewModelTest : BaseViewModelTest() {
viewModel.eventFlow.test {
viewModel.trySendAction(
EnvironmentAction.Internal.ImportKeyResultReceive(
result = ImportPrivateKeyResult.Error.UnsupportedKey,
result = ImportPrivateKeyResult.Error.UnsupportedKey(
throwable = Throwable("Fail!"),
),
),
)
assertEquals(
@@ -529,7 +531,9 @@ class EnvironmentViewModelTest : BaseViewModelTest() {
viewModel.trySendAction(
EnvironmentAction.Internal.ImportKeyResultReceive(
result = ImportPrivateKeyResult.Error.KeyStoreOperationFailed,
result = ImportPrivateKeyResult.Error.KeyStoreOperationFailed(
throwable = Throwable("Fail!"),
),
),
)
assertEquals(
@@ -539,7 +543,9 @@ class EnvironmentViewModelTest : BaseViewModelTest() {
viewModel.trySendAction(
EnvironmentAction.Internal.ImportKeyResultReceive(
result = ImportPrivateKeyResult.Error.UnrecoverableKey,
result = ImportPrivateKeyResult.Error.UnrecoverableKey(
throwable = Throwable("Fail!"),
),
),
)
assertEquals(
@@ -549,7 +555,9 @@ class EnvironmentViewModelTest : BaseViewModelTest() {
viewModel.trySendAction(
EnvironmentAction.Internal.ImportKeyResultReceive(
result = ImportPrivateKeyResult.Error.InvalidCertificateChain,
result = ImportPrivateKeyResult.Error.InvalidCertificateChain(
throwable = Throwable("Fail!"),
),
),
)
assertEquals(
@@ -589,7 +589,7 @@ class AccountSecurityViewModelTest : BaseViewModelTest() {
runTest {
coEvery {
settingsRepository.setupBiometricsKey(cipher = CIPHER)
} returns BiometricsKeyResult.Error
} returns BiometricsKeyResult.Error(error = Throwable("Fail!"))
val viewModel = createViewModel()
viewModel.stateFlow.test {