From 4a6e43983e45717694bd035518cd22cf68972521 Mon Sep 17 00:00:00 2001
From: Matthias Urhahn <matthias.urhahn@sap.com>
Date: Wed, 21 Oct 2020 08:56:11 +0200
Subject: [PATCH] Fix automatic data reset on for users affected by encryption
issues (EXPOSUREAPP-3313, EXPOSUREAPP-3335) (#1433)
* I overlooked that `KeyException` is a subclass of `GeneralSecurityException`.
This causes us to continue with `KeyException` which then fails the message matching test.
As the underlying Tink code throws the GeneralSecurityException first, we check the type of the last exception.
* Add additional specific test cases for nested exceptions.
* The dialog shouldn't automatically dismiss when the user clicks details.
They may not have read the dialog message completely or want to re-read it.
---
.../rki/coronawarnapp/ui/main/MainFragment.kt | 2 +-
.../errors/RecoveryByResetDialogFactory.kt | 17 ++--
.../util/security/EncryptionErrorResetTool.kt | 4 +-
.../util/security/EncryptionResetToolTest.kt | 82 +++++++++++++++++++
4 files changed, 95 insertions(+), 10 deletions(-)
diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/MainFragment.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/MainFragment.kt
index db82cafc9..d08f8aa4f 100644
--- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/MainFragment.kt
+++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/ui/main/MainFragment.kt
@@ -66,7 +66,7 @@ class MainFragment : Fragment(R.layout.fragment_main) {
if (errorResetTool.isResetNoticeToBeShown) {
RecoveryByResetDialogFactory(this).showDialog(
detailsLink = R.string.errors_generic_text_catastrophic_error_encryption_failure,
- onDismiss = {
+ onPositive = {
errorResetTool.isResetNoticeToBeShown = false
}
)
diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/errors/RecoveryByResetDialogFactory.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/errors/RecoveryByResetDialogFactory.kt
index da52bb3e6..c8804aaad 100644
--- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/errors/RecoveryByResetDialogFactory.kt
+++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/errors/RecoveryByResetDialogFactory.kt
@@ -14,17 +14,20 @@ class RecoveryByResetDialogFactory(private val fragment: Fragment) {
fun showDialog(
@StringRes detailsLink: Int,
- onDismiss: () -> Unit
+ onPositive: () -> Unit
) {
- AlertDialog.Builder(context)
+ val dialog = AlertDialog.Builder(context)
.setTitle(R.string.errors_generic_headline)
.setMessage(R.string.errors_generic_text_catastrophic_error_recovery_via_reset)
.setCancelable(false)
- .setOnDismissListener { onDismiss() }
- .setNeutralButton(R.string.errors_generic_button_negative) { _, _ ->
- ExternalActionHelper.openUrl(fragment, context.getString(detailsLink))
+ .setNeutralButton(R.string.errors_generic_button_negative, null)
+ .setPositiveButton(R.string.errors_generic_button_positive) { _, _ ->
+ onPositive()
}
- .setPositiveButton(R.string.errors_generic_button_positive) { _, _ -> }
- .show()
+ .create()
+ dialog.show()
+ dialog.getButton(AlertDialog.BUTTON_NEUTRAL)?.setOnClickListener {
+ ExternalActionHelper.openUrl(fragment, context.getString(detailsLink))
+ }
}
}
diff --git a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/security/EncryptionErrorResetTool.kt b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/security/EncryptionErrorResetTool.kt
index de29ea8f6..af80cfb1f 100644
--- a/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/security/EncryptionErrorResetTool.kt
+++ b/Corona-Warn-App/src/main/java/de/rki/coronawarnapp/util/security/EncryptionErrorResetTool.kt
@@ -71,8 +71,8 @@ class EncryptionErrorResetTool @Inject constructor(
}
isResetWindowConsumed = true
- val keyException = error.causes().singleOrNull { it is GeneralSecurityException }
- if (keyException == null) {
+ val keyException = error.causes().lastOrNull()
+ if (keyException == null || keyException !is GeneralSecurityException) {
Timber.v("Error has no GeneralSecurityException as cause -> no reset.")
return false
}
diff --git a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/util/security/EncryptionResetToolTest.kt b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/util/security/EncryptionResetToolTest.kt
index 7daab4ac9..d420042e1 100644
--- a/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/util/security/EncryptionResetToolTest.kt
+++ b/Corona-Warn-App/src/test/java/de/rki/coronawarnapp/util/security/EncryptionResetToolTest.kt
@@ -14,7 +14,9 @@ import org.junit.jupiter.api.Test
import testhelpers.BaseIOTest
import testhelpers.preferences.MockSharedPreferences
import java.io.File
+import java.io.IOException
import java.security.GeneralSecurityException
+import java.security.KeyException
import java.security.KeyStoreException
class EncryptionResetToolTest : BaseIOTest() {
@@ -169,6 +171,86 @@ class EncryptionResetToolTest : BaseIOTest() {
}
}
+ @Test
+ fun `nested exception may have the same base exception type, ie GeneralSecurityException`() {
+ // https://github.com/corona-warn-app/cwa-app-android/issues/642#issuecomment-712188157
+ createMockFiles()
+
+ createInstance().tryResetIfNecessary(
+ KeyException( // subclass of GeneralSecurityException
+ "Permantly failed to instantiate encrypted preferences",
+ SecurityException(
+ "Could not decrypt key. decryption failed",
+ GeneralSecurityException("decryption failed")
+ )
+ )
+ ) shouldBe true
+
+ encryptedPrefsFile.exists() shouldBe false
+ encryptedDatabaseFile.exists() shouldBe false
+
+ mockPreferences.dataMapPeek.apply {
+ this["ea1851.reset.performedAt"] shouldNotBe null
+ this["ea1851.reset.windowconsumed"] shouldBe true
+ this["ea1851.reset.shownotice"] shouldBe true
+ }
+ }
+
+ @Test
+ fun `exception check does not care about the first exception type`() {
+ createMockFiles()
+
+ createInstance().tryResetIfNecessary(
+ CwaSecurityException(
+ KeyException( // subclass of GeneralSecurityException
+ "Permantly failed to instantiate encrypted preferences",
+ SecurityException(
+ "Could not decrypt key. decryption failed",
+ GeneralSecurityException("decryption failed")
+ )
+ )
+ )
+ ) shouldBe true
+
+ encryptedPrefsFile.exists() shouldBe false
+ encryptedDatabaseFile.exists() shouldBe false
+
+ mockPreferences.dataMapPeek.apply {
+ this["ea1851.reset.performedAt"] shouldNotBe null
+ this["ea1851.reset.windowconsumed"] shouldBe true
+ this["ea1851.reset.shownotice"] shouldBe true
+ }
+ }
+
+ @Test
+ fun `exception check DOES care about the most nested exception`() {
+ createMockFiles()
+
+ createInstance().tryResetIfNecessary(
+ CwaSecurityException(
+ KeyException( // subclass of GeneralSecurityException
+ "Permantly failed to instantiate encrypted preferences",
+ SecurityException(
+ "Could not decrypt key. decryption failed",
+ GeneralSecurityException(
+ "decryption failed",
+ IOException("I am unexpeted")
+ )
+ )
+ )
+ )
+ ) shouldBe false
+
+ encryptedPrefsFile.exists() shouldBe true
+ encryptedDatabaseFile.exists() shouldBe true
+
+ mockPreferences.dataMapPeek.apply {
+ this["ea1851.reset.performedAt"] shouldBe null
+ this["ea1851.reset.windowconsumed"] shouldBe true
+ this["ea1851.reset.shownotice"] shouldBe null
+ }
+ }
+
@Test
fun `we want only a specific type of GeneralSecurityException`() {
createMockFiles()
--
GitLab