Refactor lockscreen implementation.

Try to fix issues and simplify flow.
element-hq · Aug 9, 2022 · f801ace · f801ace
1 parent 0009786
commit f801ace
Show file tree

Hide file tree

Showing 13 changed files with 185 additions and 215 deletions.
diff --git a/...androidTest/java/im/vector/app/features/pin/lockscreen/biometrics/BiometricHelperTests.kt b/...androidTest/java/im/vector/app/features/pin/lockscreen/biometrics/BiometricHelperTests.kt
@@ -31,7 +31,6 @@ import androidx.test.filters.SdkSuppress
 import androidx.test.platform.app.InstrumentationRegistry
 import im.vector.app.TestBuildVersionSdkIntProvider
 import im.vector.app.features.pin.lockscreen.configuration.LockScreenConfiguration
-import im.vector.app.features.pin.lockscreen.configuration.LockScreenConfiguratorProvider
 import im.vector.app.features.pin.lockscreen.configuration.LockScreenMode
 import im.vector.app.features.pin.lockscreen.crypto.LockScreenCryptoConstants
 import im.vector.app.features.pin.lockscreen.crypto.LockScreenKeyRepository
@@ -54,8 +53,10 @@ import kotlinx.coroutines.flow.flowOf
 import kotlinx.coroutines.flow.receiveAsFlow
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.test.runTest
+import org.amshove.kluent.coInvoking
 import org.amshove.kluent.shouldBeFalse
 import org.amshove.kluent.shouldBeTrue
+import org.amshove.kluent.shouldThrow
 import org.junit.Before
 import org.junit.Ignore
 import org.junit.Test
@@ -239,36 +240,35 @@ class BiometricHelperTests {
 
     @Test
     @SdkSuppress(minSdkVersion = Build.VERSION_CODES.R) // Due to some issues with mockk and CryptoObject initialization
-    fun authenticateCreatesSystemKeyIfNeededOnSuccessOnAndroidM() = runTest {
+    fun enableAuthenticationDeletesSystemKeyOnFailure() = runTest {
         buildVersionSdkIntProvider.value = Build.VERSION_CODES.M
-        every { lockScreenKeyRepository.isSystemKeyValid() } returns true
         val mockAuthChannel = Channel<Boolean>(capacity = 1)
         val biometricUtils = spyk(createBiometricHelper(createDefaultConfiguration(isBiometricsEnabled = true))) {
             every { createAuthChannel() } returns mockAuthChannel
             every { authenticateWithPromptInternal(any(), any(), any()) } returns mockk()
         }
+        every { lockScreenKeyRepository.deleteSystemKey() } returns Unit
 
         val latch = CountDownLatch(1)
         val intent = Intent(InstrumentationRegistry.getInstrumentation().targetContext, LockScreenTestActivity::class.java)
         ActivityScenario.launch<LockScreenTestActivity>(intent).onActivity { activity ->
             activity.lifecycleScope.launch {
+                val exception = IllegalStateException("Some error")
                 launch {
-                    mockAuthChannel.send(true)
-                    mockAuthChannel.close()
+                    mockAuthChannel.close(exception)
                 }
-                biometricUtils.authenticate(activity).collect()
+                coInvoking { biometricUtils.enableAuthentication(activity).collect() } shouldThrow exception
                 latch.countDown()
             }
         }
 
         latch.await(1, TimeUnit.SECONDS)
-        verify { lockScreenKeyRepository.ensureSystemKey() }
+        verify { lockScreenKeyRepository.deleteSystemKey() }
     }
 
     private fun createBiometricHelper(configuration: LockScreenConfiguration): BiometricHelper {
         val context = InstrumentationRegistry.getInstrumentation().targetContext
-        val configProvider = LockScreenConfiguratorProvider(configuration)
-        return BiometricHelper(context, lockScreenKeyRepository, configProvider, biometricManager, buildVersionSdkIntProvider)
+        return BiometricHelper(configuration, context, lockScreenKeyRepository, biometricManager, buildVersionSdkIntProvider)
     }
 
     private fun createDefaultConfiguration(

diff --git a/...oidTest/java/im/vector/app/features/pin/lockscreen/crypto/LockScreenKeyRepositoryTests.kt b/...oidTest/java/im/vector/app/features/pin/lockscreen/crypto/LockScreenKeyRepositoryTests.kt
@@ -17,8 +17,6 @@
 package im.vector.app.features.pin.lockscreen.crypto
 
 import androidx.test.platform.app.InstrumentationRegistry
-import im.vector.app.features.pin.lockscreen.crypto.migrations.LegacyPinCodeMigrator
-import im.vector.app.features.settings.VectorPreferences
 import io.mockk.clearAllMocks
 import io.mockk.every
 import io.mockk.mockk
@@ -44,8 +42,6 @@ class LockScreenKeyRepositoryTests {
     }
 
     private lateinit var lockScreenKeyRepository: LockScreenKeyRepository
-    private val legacyPinCodeMigrator: LegacyPinCodeMigrator = mockk(relaxed = true)
-    private val vectorPreferences: VectorPreferences = mockk(relaxed = true)
 
     private val keyStore: KeyStore by lazy {
         KeyStore.getInstance(LockScreenCryptoConstants.ANDROID_KEY_STORE).also { it.load(null) }

diff --git a/vector/src/main/java/im/vector/app/features/pin/PinFragment.kt b/vector/src/main/java/im/vector/app/features/pin/PinFragment.kt
@@ -24,6 +24,7 @@ import android.view.View
 import android.view.ViewGroup
 import android.widget.Toast
 import com.airbnb.mvrx.args
+import com.airbnb.mvrx.asMavericksArgs
 import com.google.android.material.dialog.MaterialAlertDialogBuilder
 import im.vector.app.R
 import im.vector.app.core.extensions.replaceFragment
@@ -33,7 +34,7 @@ import im.vector.app.databinding.FragmentPinBinding
 import im.vector.app.features.MainActivity
 import im.vector.app.features.MainActivityArgs
 import im.vector.app.features.pin.lockscreen.biometrics.BiometricAuthError
-import im.vector.app.features.pin.lockscreen.configuration.LockScreenConfiguratorProvider
+import im.vector.app.features.pin.lockscreen.configuration.LockScreenConfiguration
 import im.vector.app.features.pin.lockscreen.configuration.LockScreenMode
 import im.vector.app.features.pin.lockscreen.ui.AuthMethod
 import im.vector.app.features.pin.lockscreen.ui.LockScreenFragment
@@ -51,7 +52,7 @@ data class PinArgs(
 class PinFragment @Inject constructor(
         private val pinCodeStore: PinCodeStore,
         private val vectorPreferences: VectorPreferences,
-        private val configuratorProvider: LockScreenConfiguratorProvider,
+        private val defaultConfiguration: LockScreenConfiguration,
 ) : VectorBaseFragment<FragmentPinBinding>() {
 
     private val fragmentArgs: PinArgs by args()
@@ -81,21 +82,17 @@ class PinFragment @Inject constructor(
                 vectorBaseActivity.finish()
             }
         }
-
-        configuratorProvider.updateDefaultConfiguration {
-            copy(
-                    mode = LockScreenMode.CREATE,
-                    title = getString(R.string.create_pin_title),
-                    needsNewCodeValidation = true,
-                    newCodeConfirmationTitle = getString(R.string.create_pin_confirm_title),
-            )
-        }
+        createFragment.arguments = defaultConfiguration.copy(
+                mode = LockScreenMode.CREATE,
+                title = getString(R.string.create_pin_title),
+                needsNewCodeValidation = true,
+                newCodeConfirmationTitle = getString(R.string.create_pin_confirm_title),
+        ).asMavericksArgs()
         replaceFragment(R.id.pinFragmentContainer, createFragment)
     }
 
     private fun showAuthFragment() {
         val authFragment = LockScreenFragment()
-        val canUseBiometrics = vectorPreferences.useBiometricsToUnlock()
         authFragment.onLeftButtonClickedListener = View.OnClickListener { displayForgotPinWarningDialog() }
         authFragment.lockScreenListener = object : LockScreenListener {
             override fun onAuthenticationFailure(authMethod: AuthMethod) {
@@ -133,18 +130,12 @@ class PinFragment @Inject constructor(
                         .show()
             }
         }
-        configuratorProvider.updateDefaultConfiguration {
-            copy(
-                    mode = LockScreenMode.VERIFY,
-                    title = getString(R.string.auth_pin_title),
-                    isStrongBiometricsEnabled = isStrongBiometricsEnabled && canUseBiometrics,
-                    isWeakBiometricsEnabled = isWeakBiometricsEnabled && canUseBiometrics,
-                    isDeviceCredentialUnlockEnabled = isDeviceCredentialUnlockEnabled && canUseBiometrics,
-                    autoStartBiometric = canUseBiometrics,
-                    leftButtonTitle = getString(R.string.auth_pin_forgot),
-                    clearCodeOnError = true,
-            )
-        }
+        authFragment.arguments = defaultConfiguration.copy(
+                mode = LockScreenMode.VERIFY,
+                title = getString(R.string.auth_pin_title),
+                leftButtonTitle = getString(R.string.auth_pin_forgot),
+                clearCodeOnError = true,
+        ).asMavericksArgs()
         replaceFragment(R.id.pinFragmentContainer, authFragment)
     }
 

diff --git a/vector/src/main/java/im/vector/app/features/pin/lockscreen/biometrics/BiometricHelper.kt b/vector/src/main/java/im/vector/app/features/pin/lockscreen/biometrics/BiometricHelper.kt
@@ -31,10 +31,12 @@ import androidx.biometric.BiometricPrompt
 import androidx.core.content.ContextCompat
 import androidx.fragment.app.Fragment
 import androidx.fragment.app.FragmentActivity
+import dagger.assisted.Assisted
+import dagger.assisted.AssistedFactory
+import dagger.assisted.AssistedInject
 import dagger.hilt.android.qualifiers.ApplicationContext
 import im.vector.app.R
 import im.vector.app.features.pin.lockscreen.configuration.LockScreenConfiguration
-import im.vector.app.features.pin.lockscreen.configuration.LockScreenConfiguratorProvider
 import im.vector.app.features.pin.lockscreen.crypto.LockScreenKeyRepository
 import im.vector.app.features.pin.lockscreen.ui.fallbackprompt.FallbackBiometricDialogFragment
 import im.vector.app.features.pin.lockscreen.utils.DevicePromptCheck
@@ -54,22 +56,26 @@ import kotlinx.coroutines.launch
 import org.matrix.android.sdk.api.util.BuildVersionSdkIntProvider
 import java.security.KeyStore
 import javax.crypto.Cipher
-import javax.inject.Inject
 import kotlin.coroutines.CoroutineContext
 
 /**
  * This is a helper to manage system authentication (biometric and other types) and the system key.
  */
-class BiometricHelper @Inject constructor(
+class BiometricHelper @AssistedInject constructor(
+        @Assisted private val configuration: LockScreenConfiguration,
         @ApplicationContext private val context: Context,
         private val lockScreenKeyRepository: LockScreenKeyRepository,
-        private val configurationProvider: LockScreenConfiguratorProvider,
         private val biometricManager: BiometricManager,
         private val buildVersionSdkIntProvider: BuildVersionSdkIntProvider,
 ) {
     private var prompt: BiometricPrompt? = null
 
-    private val configuration: LockScreenConfiguration get() = configurationProvider.currentConfiguration
+    @AssistedFactory
+    interface BiometricHelperFactory {
+        fun create(configuration: LockScreenConfiguration): BiometricHelper
+    }
+
+//    private val configuration: LockScreenConfiguration get() = configurationProvider.currentConfiguration
 
     /**
      * Returns true if a weak biometric method (i.e.: some face or iris unlock implementations) can be used.
@@ -174,16 +180,18 @@ class BiometricHelper @Inject constructor(
                 when (val exception = result.exceptionOrNull()) {
                     null -> result.getOrNull()?.let { emit(it) }
                     else -> {
-                        // Exception found, stop collecting, throw it and remove the prompt reference
+                        // Exception found:
+                        // 1. Stop collecting.
+                        // 2. Remove the system key if we were creating it.
+                        // 3. Throw the exception and remove the prompt reference
+                        if (!checkSystemKeyExists) {
+                            lockScreenKeyRepository.deleteSystemKey()
+                        }
                         prompt = null
                         throw exception
                     }
                 }
             }
-            // Generates the system key on successful authentication
-            if (buildVersionSdkIntProvider.get() >= Build.VERSION_CODES.M) {
-                lockScreenKeyRepository.ensureSystemKey()
-            }
             // Channel is closed, remove prompt reference
             prompt = null
         }
@@ -213,11 +221,11 @@ class BiometricHelper @Inject constructor(
                 .setAllowedAuthenticators(authenticators)
                 .build()
 
-        return BiometricPrompt(activity, executor, callback).also {
+        return BiometricPrompt(activity, executor, callback).also { prompt ->
             showFallbackFragmentIfNeeded(activity, channel.receiveAsFlow(), executor.asCoroutineDispatcher()) {
                 // For some reason this seems to be needed unless we want to receive a fragment transaction exception
                 delay(1L)
-                it.authenticate(promptInfo, cryptoObject)
+                prompt.authenticate(promptInfo, cryptoObject)
             }
         }
     }
@@ -253,11 +261,9 @@ class BiometricHelper @Inject constructor(
     ): BiometricPrompt.AuthenticationCallback = object : BiometricPrompt.AuthenticationCallback() {
         private val scope = CoroutineScope(coroutineContext)
         override fun onAuthenticationError(errorCode: Int, errString: CharSequence) {
-            scope.launch {
-                // Error is a terminal event, should close both the Channel and the CoroutineScope to free resources.
-                channel.close(BiometricAuthError(errorCode, errString.toString()))
-                scope.cancel()
-            }
+            // Error is a terminal event, should close both the Channel and the CoroutineScope to free resources.
+            channel.close(BiometricAuthError(errorCode, errString.toString()))
+            scope.cancel()
         }
 
         override fun onAuthenticationFailed() {
@@ -274,10 +280,8 @@ class BiometricHelper @Inject constructor(
                     scope.cancel()
                 }
             } else {
-                scope.launch {
-                    channel.close(IllegalStateException("System key was not valid after authentication."))
-                    scope.cancel()
-                }
+                channel.close(IllegalStateException("System key was not valid after authentication."))
+                scope.cancel()
             }
         }
 

diff --git a/.../main/java/im/vector/app/features/pin/lockscreen/configuration/LockScreenConfiguration.kt b/.../main/java/im/vector/app/features/pin/lockscreen/configuration/LockScreenConfiguration.kt
@@ -16,9 +16,13 @@
 
 package im.vector.app.features.pin.lockscreen.configuration
 
+import android.os.Parcelable
+import kotlinx.parcelize.Parcelize
+
 /**
  * Configuration to be used by the lockscreen feature.
  */
+@Parcelize
 data class LockScreenConfiguration(
         /** Which mode should the UI display, [LockScreenMode.VERIFY] or [LockScreenMode.CREATE]. */
         val mode: LockScreenMode,
@@ -56,4 +60,4 @@ data class LockScreenConfiguration(
         val biometricSubtitle: String? = null,
         /** Text for the cancel button of the Biometric prompt dialog. Optional. */
         val biometricCancelButtonTitle: String? = null,
-)
+) : Parcelable
diff --git a/...ava/im/vector/app/features/pin/lockscreen/configuration/LockScreenConfiguratorProvider.kt b/...ava/im/vector/app/features/pin/lockscreen/configuration/LockScreenConfiguratorProvider.kt
diff --git a/vector/src/main/java/im/vector/app/features/pin/lockscreen/di/LockScreenModule.kt b/vector/src/main/java/im/vector/app/features/pin/lockscreen/di/LockScreenModule.kt
@@ -16,8 +16,10 @@
 
 package im.vector.app.features.pin.lockscreen.di
 
+import android.app.KeyguardManager
 import android.content.Context
 import androidx.biometric.BiometricManager
+import androidx.core.content.getSystemService
 import dagger.Binds
 import dagger.Module
 import dagger.Provides
@@ -83,6 +85,9 @@ object LockScreenModule {
             SecretStoringUtils(context, keyStore, buildVersionSdkIntProvider),
             buildVersionSdkIntProvider,
     )
+
+    @Provides
+    fun provideKeyguardManager(context: Context): KeyguardManager = context.getSystemService()!!
 }
 
 @Module