From c848615636465b1ffcecb44134ca16fa063297be Mon Sep 17 00:00:00 2001 From: Jorge Martin Espinosa Date: Tue, 2 Aug 2022 15:31:08 +0200 Subject: [PATCH] Disable 'Enable biometrics' option if there are not biometric authenticators enrolled. (#6714) * Disable 'Enable biometrics' option if there are not biometric authenticators enrolled. * Improve biometric pref enabled check * Fix changelog issue * Address review comments. Add extra catch clauses to key migrations. * Add tests for key migrators --- changelog.d/6713.bugfix | 1 + .../pin/lockscreen/crypto/KeyStoreCryptoTests.kt | 9 ++++++--- .../pin/lockscreen/crypto/KeyStoreCrypto.kt | 3 +++ .../migrations/MissingSystemKeyMigrator.kt | 5 ++++- .../crypto/migrations/SystemKeyV1Migrator.kt | 8 +++++++- .../pin/lockscreen/ui/LockScreenFragment.kt | 2 +- .../settings/VectorSettingsPinFragment.kt | 16 +++++++--------- .../migrations/MissingSystemKeyMigratorTests.kt | 12 ++++++++++++ .../migrations/SystemKeyV1MigratorTests.kt | 16 ++++++++++++++++ 9 files changed, 57 insertions(+), 15 deletions(-) create mode 100644 changelog.d/6713.bugfix diff --git a/changelog.d/6713.bugfix b/changelog.d/6713.bugfix new file mode 100644 index 0000000000..6a9926aa13 --- /dev/null +++ b/changelog.d/6713.bugfix @@ -0,0 +1 @@ +Disable 'Enable biometrics' option if there are not biometric authenticators enrolled. diff --git a/vector/src/androidTest/java/im/vector/app/features/pin/lockscreen/crypto/KeyStoreCryptoTests.kt b/vector/src/androidTest/java/im/vector/app/features/pin/lockscreen/crypto/KeyStoreCryptoTests.kt index 6e02cc0262..1712ec889e 100644 --- a/vector/src/androidTest/java/im/vector/app/features/pin/lockscreen/crypto/KeyStoreCryptoTests.kt +++ b/vector/src/androidTest/java/im/vector/app/features/pin/lockscreen/crypto/KeyStoreCryptoTests.kt @@ -18,6 +18,7 @@ package im.vector.app.features.pin.lockscreen.crypto import android.os.Build import android.security.keystore.KeyPermanentlyInvalidatedException +import android.security.keystore.UserNotAuthenticatedException import androidx.test.platform.app.InstrumentationRegistry import im.vector.app.TestBuildVersionSdkIntProvider import io.mockk.every @@ -69,10 +70,12 @@ class KeyStoreCryptoTests { runCatching { keyStoreCrypto.ensureKey() } keyStoreCrypto.hasValidKey() shouldBe true - val exception = KeyPermanentlyInvalidatedException() - every { secretStoringUtils.getEncryptCipher(any()) } throws exception + val keyInvalidatedException = KeyPermanentlyInvalidatedException() + every { secretStoringUtils.getEncryptCipher(any()) } throws keyInvalidatedException + keyStoreCrypto.hasValidKey() shouldBe false - runCatching { keyStoreCrypto.ensureKey() } + val userNotAuthenticatedException = UserNotAuthenticatedException() + every { secretStoringUtils.getEncryptCipher(any()) } throws userNotAuthenticatedException keyStoreCrypto.hasValidKey() shouldBe false } diff --git a/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/KeyStoreCrypto.kt b/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/KeyStoreCrypto.kt index 4c52fccbaa..d37c11ed69 100644 --- a/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/KeyStoreCrypto.kt +++ b/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/KeyStoreCrypto.kt @@ -20,6 +20,7 @@ import android.annotation.SuppressLint import android.content.Context import android.os.Build import android.security.keystore.KeyPermanentlyInvalidatedException +import android.security.keystore.UserNotAuthenticatedException import android.util.Base64 import androidx.annotation.VisibleForTesting import androidx.biometric.BiometricPrompt @@ -117,6 +118,8 @@ class KeyStoreCrypto @AssistedInject constructor( true } catch (e: KeyPermanentlyInvalidatedException) { false + } catch (e: UserNotAuthenticatedException) { + false } } else { keyExists diff --git a/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigrator.kt b/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigrator.kt index f8d3520047..75a68c66b7 100644 --- a/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigrator.kt +++ b/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigrator.kt @@ -19,6 +19,7 @@ package im.vector.app.features.pin.lockscreen.crypto.migrations import android.annotation.SuppressLint import android.os.Build import android.security.keystore.KeyPermanentlyInvalidatedException +import android.security.keystore.UserNotAuthenticatedException import im.vector.app.features.pin.lockscreen.crypto.KeyStoreCrypto import im.vector.app.features.pin.lockscreen.di.BiometricKeyAlias import im.vector.app.features.settings.VectorPreferences @@ -45,7 +46,9 @@ class MissingSystemKeyMigrator @Inject constructor( try { keystoreCryptoFactory.provide(systemKeyAlias, true).ensureKey() } catch (e: KeyPermanentlyInvalidatedException) { - Timber.e("Could not automatically create biometric key.", e) + Timber.e("Could not automatically create biometric key because it was invalidated.", e) + } catch (e: UserNotAuthenticatedException) { + Timber.e("Could not automatically create biometric key because there are no enrolled biometric authenticators.", e) } } } diff --git a/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1Migrator.kt b/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1Migrator.kt index 25e9d24272..10c7505f27 100644 --- a/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1Migrator.kt +++ b/vector/src/main/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1Migrator.kt @@ -17,9 +17,11 @@ package im.vector.app.features.pin.lockscreen.crypto.migrations import android.os.Build +import android.security.keystore.UserNotAuthenticatedException import androidx.annotation.RequiresApi import im.vector.app.features.pin.lockscreen.crypto.KeyStoreCrypto import im.vector.app.features.pin.lockscreen.di.BiometricKeyAlias +import timber.log.Timber import java.security.KeyStore import javax.inject.Inject @@ -39,7 +41,11 @@ class SystemKeyV1Migrator @Inject constructor( fun migrate() { keyStore.deleteEntry(SYSTEM_KEY_ALIAS_V1) val systemKeyStoreCrypto = keystoreCryptoFactory.provide(systemKeyAlias, keyNeedsUserAuthentication = true) - systemKeyStoreCrypto.ensureKey() + try { + systemKeyStoreCrypto.ensureKey() + } catch (e: UserNotAuthenticatedException) { + Timber.e("Could not migrate v1 biometric key because there are no enrolled biometric authenticators.", e) + } } /** diff --git a/vector/src/main/java/im/vector/app/features/pin/lockscreen/ui/LockScreenFragment.kt b/vector/src/main/java/im/vector/app/features/pin/lockscreen/ui/LockScreenFragment.kt index 0e6fdfb61e..9eea61ac82 100644 --- a/vector/src/main/java/im/vector/app/features/pin/lockscreen/ui/LockScreenFragment.kt +++ b/vector/src/main/java/im/vector/app/features/pin/lockscreen/ui/LockScreenFragment.kt @@ -59,7 +59,7 @@ class LockScreenFragment : VectorBaseFragment() { if (state.lockScreenConfiguration.mode == LockScreenMode.CREATE) return@withState viewLifecycleOwner.lifecycleScope.launchWhenResumed { - if (state.isBiometricKeyInvalidated) { + if (state.canUseBiometricAuth && state.isBiometricKeyInvalidated) { lockScreenListener?.onBiometricKeyInvalidated() } else if (state.showBiometricPromptAutomatically) { showBiometricPrompt() diff --git a/vector/src/main/java/im/vector/app/features/settings/VectorSettingsPinFragment.kt b/vector/src/main/java/im/vector/app/features/settings/VectorSettingsPinFragment.kt index 087ee18afa..4d95fc362b 100644 --- a/vector/src/main/java/im/vector/app/features/settings/VectorSettingsPinFragment.kt +++ b/vector/src/main/java/im/vector/app/features/settings/VectorSettingsPinFragment.kt @@ -60,17 +60,16 @@ class VectorSettingsPinFragment @Inject constructor( findPreference(VectorPreferences.SETTINGS_SECURITY_USE_BIOMETRICS_FLAG)!! } - private fun shouldCheckBiometricPref(isPinCodeChecked: Boolean): Boolean { - return isPinCodeChecked && // Biometric auth depends on PIN auth - biometricHelper.isSystemAuthEnabledAndValid && - biometricHelper.isSystemKeyValid + private fun updateBiometricPrefState(isPinCodeChecked: Boolean) { + // Biometric auth depends on PIN auth + useBiometricPref.isEnabled = isPinCodeChecked && biometricHelper.canUseAnySystemAuth + useBiometricPref.isChecked = isPinCodeChecked && biometricHelper.isSystemAuthEnabledAndValid } override fun onResume() { super.onResume() - useBiometricPref.isEnabled = usePinCodePref.isChecked - useBiometricPref.isChecked = shouldCheckBiometricPref(usePinCodePref.isChecked) + updateBiometricPrefState(isPinCodeChecked = usePinCodePref.isChecked) } override fun bindPref() { @@ -78,8 +77,7 @@ class VectorSettingsPinFragment @Inject constructor( usePinCodePref.setOnPreferenceChangeListener { _, value -> val isChecked = (value as? Boolean).orFalse() - useBiometricPref.isEnabled = isChecked - useBiometricPref.isChecked = shouldCheckBiometricPref(isChecked) + updateBiometricPrefState(isPinCodeChecked = isChecked) if (!isChecked) { disableBiometricAuthentication() } @@ -104,7 +102,7 @@ class VectorSettingsPinFragment @Inject constructor( }.onFailure { showEnableBiometricErrorMessage() } - useBiometricPref.isChecked = shouldCheckBiometricPref(usePinCodePref.isChecked) + updateBiometricPrefState(isPinCodeChecked = usePinCodePref.isChecked) } false } else { diff --git a/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigratorTests.kt b/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigratorTests.kt index d65c3da2d2..3098187962 100644 --- a/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigratorTests.kt +++ b/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/MissingSystemKeyMigratorTests.kt @@ -18,6 +18,7 @@ package im.vector.app.features.pin.lockscreen.crypto.migrations import android.os.Build import android.security.keystore.KeyPermanentlyInvalidatedException +import android.security.keystore.UserNotAuthenticatedException import im.vector.app.features.pin.lockscreen.crypto.KeyStoreCrypto import im.vector.app.features.settings.VectorPreferences import im.vector.app.test.TestBuildVersionSdkIntProvider @@ -59,6 +60,17 @@ class MissingSystemKeyMigratorTests { invoking { missingSystemKeyMigrator.migrate() } shouldNotThrow KeyPermanentlyInvalidatedException::class } + @Test + fun migrateHandlesUserNotAuthenticatedExceptions() { + val keyStoreCryptoMock = mockk { + every { ensureKey() } throws UserNotAuthenticatedException() + } + every { keyStoreCryptoFactory.provide(any(), any()) } returns keyStoreCryptoMock + every { vectorPreferences.useBiometricsToUnlock() } returns true + + invoking { missingSystemKeyMigrator.migrate() } shouldNotThrow UserNotAuthenticatedException::class + } + @Test fun migrateReturnsEarlyIfBiometricAuthIsDisabled() { val keyStoreCryptoMock = mockk { diff --git a/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1MigratorTests.kt b/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1MigratorTests.kt index a519251398..5cbb828f71 100644 --- a/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1MigratorTests.kt +++ b/vector/src/test/java/im/vector/app/features/pin/lockscreen/crypto/migrations/SystemKeyV1MigratorTests.kt @@ -16,11 +16,14 @@ package im.vector.app.features.pin.lockscreen.crypto.migrations +import android.security.keystore.UserNotAuthenticatedException import im.vector.app.features.pin.lockscreen.crypto.KeyStoreCrypto import io.mockk.every import io.mockk.mockk import io.mockk.verify +import org.amshove.kluent.invoking import org.amshove.kluent.shouldBe +import org.amshove.kluent.shouldNotThrow import org.junit.Test import java.security.KeyStore @@ -39,6 +42,19 @@ class SystemKeyV1MigratorTests { systemKeyV1Migrator.isMigrationNeeded() shouldBe false } + @Test + fun migrateHandlesUserNotAuthenticatedException() { + val keyStoreCryptoMock = mockk { + every { ensureKey() } throws UserNotAuthenticatedException() + } + every { keyStoreCryptoFactory.provide("vector.system_new", any()) } returns keyStoreCryptoMock + + invoking { systemKeyV1Migrator.migrate() } shouldNotThrow UserNotAuthenticatedException::class + + verify { keyStore.deleteEntry(SystemKeyV1Migrator.SYSTEM_KEY_ALIAS_V1) } + verify { keyStoreCryptoMock.ensureKey() } + } + @Test fun migrateDeletesOldEntryAndEnsuresNewKey() { val keyStoreCryptoMock = mockk {