From 8baf414d8194d2827e0ca2cfeae5520272fdf3a4 Mon Sep 17 00:00:00 2001 From: Matthias Urhahn Date: Mon, 19 Oct 2020 18:44:48 +0200 Subject: [PATCH 1/3] 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. --- .../util/security/EncryptionErrorResetTool.kt | 4 +-- .../util/security/EncryptionResetToolTest.kt | 28 +++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) 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 de29ea8f681..af80cfb1f15 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 7daab4ac948..7d25cb018f6 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 @@ -15,6 +15,7 @@ import testhelpers.BaseIOTest import testhelpers.preferences.MockSharedPreferences import java.io.File import java.security.GeneralSecurityException +import java.security.KeyException import java.security.KeyStoreException class EncryptionResetToolTest : BaseIOTest() { @@ -169,6 +170,33 @@ class EncryptionResetToolTest : BaseIOTest() { } } + @Test + fun `nested exception may have the same base exception type, GeneralSecurityException`() { + // https://github.com/corona-warn-app/cwa-app-android/issues/642#issuecomment-712188157 + 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 `we want only a specific type of GeneralSecurityException`() { createMockFiles() From 557adc47f7a0f405ca991fd6ab5b16d7ddb85b20 Mon Sep 17 00:00:00 2001 From: Matthias Urhahn Date: Tue, 20 Oct 2020 16:29:36 +0200 Subject: [PATCH 2/3] Add additional specific test cases for nested exceptions. --- .../util/security/EncryptionResetToolTest.kt | 56 ++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) 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 7d25cb018f6..d420042e18e 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,6 +14,7 @@ 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 @@ -171,10 +172,34 @@ class EncryptionResetToolTest : BaseIOTest() { } @Test - fun `nested exception may have the same base exception type, GeneralSecurityException`() { + 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 @@ -197,6 +222,35 @@ class EncryptionResetToolTest : BaseIOTest() { } } + @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() From e4e2280734e896dae4cfe354a573792a11cd0a01 Mon Sep 17 00:00:00 2001 From: Matthias Urhahn Date: Tue, 20 Oct 2020 16:30:53 +0200 Subject: [PATCH 3/3] 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 +- .../util/errors/RecoveryByResetDialogFactory.kt | 17 ++++++++++------- 2 files changed, 11 insertions(+), 8 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 db82cafc9c5..d08f8aa4f30 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 da52bb3e6e5..c8804aaad1d 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)) + } } }