From f10231091dc28ace504a149676f96ae017c778c6 Mon Sep 17 00:00:00 2001 From: ganfra Date: Tue, 7 Nov 2023 15:55:53 +0100 Subject: [PATCH 1/9] LockScreen : refact some code and add secureFlag --- .../io/element/android/x/MainActivity.kt | 2 + .../io/element/android/x/di/AppBindings.kt | 2 + .../ftue/impl/state/DefaultFtueState.kt | 2 +- .../ftue/impl/DefaultFtueStateTests.kt | 27 +++++++++++--- .../lockscreen/api/LockScreenService.kt | 37 ++++++++++++++++++- .../impl/DefaultLockScreenService.kt | 23 +++++++++--- .../impl/pin/DefaultPinCodeManager.kt | 3 +- .../lockscreen/impl/pin/PinCodeManager.kt | 4 +- .../settings/LockScreenSettingsFlowNode.kt | 7 +++- .../settings/LockScreenSettingsPresenter.kt | 22 +++-------- .../impl/storage/EncryptedPinCodeStorage.kt | 5 ++- .../storage/PreferencesLockScreenStore.kt | 4 +- .../impl/pin/DefaultPinCodeManagerTest.kt | 12 ++++-- .../pin/storage/InMemoryLockScreenStore.kt | 9 ++++- .../lockscreen/test/FakeLockScreenService.kt | 16 +++++--- 15 files changed, 128 insertions(+), 47 deletions(-) diff --git a/app/src/main/kotlin/io/element/android/x/MainActivity.kt b/app/src/main/kotlin/io/element/android/x/MainActivity.kt index 7109743052..7fcae7040e 100644 --- a/app/src/main/kotlin/io/element/android/x/MainActivity.kt +++ b/app/src/main/kotlin/io/element/android/x/MainActivity.kt @@ -32,6 +32,7 @@ import androidx.core.view.WindowCompat import com.bumble.appyx.core.integration.NodeHost import com.bumble.appyx.core.integrationpoint.NodeActivity import com.bumble.appyx.core.plugin.NodeReadyObserver +import io.element.android.features.lockscreen.api.handleSecureFlag import io.element.android.libraries.architecture.bindings import io.element.android.libraries.core.log.logger.LoggerTag import io.element.android.libraries.designsystem.utils.snackbar.LocalSnackbarDispatcher @@ -53,6 +54,7 @@ class MainActivity : NodeActivity() { installSplashScreen() super.onCreate(savedInstanceState) appBindings = bindings() + appBindings.lockScreenService().handleSecureFlag(this) WindowCompat.setDecorFitsSystemWindows(window, false) setContent { MainContent(appBindings) diff --git a/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt b/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt index 185b3834c5..512b5093d0 100644 --- a/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt +++ b/app/src/main/kotlin/io/element/android/x/di/AppBindings.kt @@ -17,6 +17,7 @@ package io.element.android.x.di import com.squareup.anvil.annotations.ContributesTo +import io.element.android.features.lockscreen.api.LockScreenService import io.element.android.features.rageshake.api.reporter.BugReporter import io.element.android.libraries.designsystem.utils.snackbar.SnackbarDispatcher import io.element.android.libraries.di.AppScope @@ -27,4 +28,5 @@ interface AppBindings { fun snackbarDispatcher(): SnackbarDispatcher fun tracingService(): TracingService fun bugReporter(): BugReporter + fun lockScreenService(): LockScreenService } diff --git a/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueState.kt b/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueState.kt index 7d80fd7413..b1abee90bc 100644 --- a/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueState.kt +++ b/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueState.kt @@ -120,7 +120,7 @@ class DefaultFtueState @Inject constructor( private fun shouldDisplayLockscreenSetup(): Boolean { return runBlocking { - lockScreenService.isSetupRequired() + lockScreenService.isSetupRequired().first() } } diff --git a/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueStateTests.kt b/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueStateTests.kt index b84bd93d3b..13de50c6b4 100644 --- a/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueStateTests.kt +++ b/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueStateTests.kt @@ -57,6 +57,7 @@ class DefaultFtueStateTests { val analyticsService = FakeAnalyticsService() val migrationScreenStore = InMemoryMigrationScreenStore() val permissionStateProvider = FakePermissionStateProvider(permissionGranted = true) + val lockScreenService = FakeLockScreenService() val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val state = createState( @@ -64,13 +65,15 @@ class DefaultFtueStateTests { welcomeState = welcomeState, analyticsService = analyticsService, migrationScreenStore = migrationScreenStore, - permissionStateProvider = permissionStateProvider + permissionStateProvider = permissionStateProvider, + lockScreenService = lockScreenService, ) welcomeState.setWelcomeScreenShown() analyticsService.setDidAskUserConsent() migrationScreenStore.setMigrationScreenShown(A_SESSION_ID) permissionStateProvider.setPermissionGranted() + lockScreenService.setIsPinSetup(true) state.updateState() assertThat(state.shouldDisplayFlow.value).isFalse() @@ -85,6 +88,7 @@ class DefaultFtueStateTests { val analyticsService = FakeAnalyticsService() val migrationScreenStore = InMemoryMigrationScreenStore() val permissionStateProvider = FakePermissionStateProvider(permissionGranted = false) + val lockScreenService = FakeLockScreenService() val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val state = createState( @@ -92,7 +96,8 @@ class DefaultFtueStateTests { welcomeState = welcomeState, analyticsService = analyticsService, migrationScreenStore = migrationScreenStore, - permissionStateProvider = permissionStateProvider + permissionStateProvider = permissionStateProvider, + lockScreenService = lockScreenService, ) val steps = mutableListOf() @@ -108,7 +113,11 @@ class DefaultFtueStateTests { steps.add(state.getNextStep(steps.lastOrNull())) permissionStateProvider.setPermissionGranted() - // Fourth step, analytics opt in + // Fourth step, notifications opt in + steps.add(state.getNextStep(steps.lastOrNull())) + lockScreenService.setIsPinSetup(true) + + // Fifth step, analytics opt in steps.add(state.getNextStep(steps.lastOrNull())) analyticsService.setDidAskUserConsent() @@ -119,6 +128,7 @@ class DefaultFtueStateTests { FtueStep.MigrationScreen, FtueStep.WelcomeScreen, FtueStep.NotificationsOptIn, + FtueStep.LockscreenSetup, FtueStep.AnalyticsOptIn, null, // Final state ) @@ -133,18 +143,20 @@ class DefaultFtueStateTests { val analyticsService = FakeAnalyticsService() val migrationScreenStore = InMemoryMigrationScreenStore() val permissionStateProvider = FakePermissionStateProvider(permissionGranted = false) - + val lockScreenService = FakeLockScreenService() val state = createState( coroutineScope = coroutineScope, analyticsService = analyticsService, migrationScreenStore = migrationScreenStore, permissionStateProvider = permissionStateProvider, + lockScreenService = lockScreenService, ) - // Skip first 3 steps + // Skip first 4 steps migrationScreenStore.setMigrationScreenShown(A_SESSION_ID) state.setWelcomeScreenShown() permissionStateProvider.setPermissionGranted() + lockScreenService.setIsPinSetup(true) assertThat(state.getNextStep()).isEqualTo(FtueStep.AnalyticsOptIn) @@ -160,18 +172,21 @@ class DefaultFtueStateTests { val coroutineScope = CoroutineScope(coroutineContext + SupervisorJob()) val analyticsService = FakeAnalyticsService() val migrationScreenStore = InMemoryMigrationScreenStore() + val lockScreenService = FakeLockScreenService() val state = createState( sdkIntVersion = Build.VERSION_CODES.M, coroutineScope = coroutineScope, analyticsService = analyticsService, migrationScreenStore = migrationScreenStore, + lockScreenService = lockScreenService, ) migrationScreenStore.setMigrationScreenShown(A_SESSION_ID) assertThat(state.getNextStep()).isEqualTo(FtueStep.WelcomeScreen) - state.setWelcomeScreenShown() + lockScreenService.setIsPinSetup(true) + assertThat(state.getNextStep()).isEqualTo(FtueStep.AnalyticsOptIn) analyticsService.setDidAskUserConsent() diff --git a/features/lockscreen/api/src/main/kotlin/io/element/android/features/lockscreen/api/LockScreenService.kt b/features/lockscreen/api/src/main/kotlin/io/element/android/features/lockscreen/api/LockScreenService.kt index c6fe444119..e0fe432ede 100644 --- a/features/lockscreen/api/src/main/kotlin/io/element/android/features/lockscreen/api/LockScreenService.kt +++ b/features/lockscreen/api/src/main/kotlin/io/element/android/features/lockscreen/api/LockScreenService.kt @@ -16,7 +16,14 @@ package io.element.android.features.lockscreen.api +import android.os.Build +import android.view.WindowManager +import androidx.activity.ComponentActivity +import androidx.lifecycle.lifecycleScope +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.onEach interface LockScreenService { /** @@ -28,5 +35,33 @@ interface LockScreenService { * Check if setting up the lock screen is required. * @return true if the lock screen is mandatory and not setup yet, false otherwise. */ - suspend fun isSetupRequired(): Boolean + fun isSetupRequired(): Flow + + /** + * Check if pin is setup. + * @return true if the pin is setup, false otherwise. + */ + fun isPinSetup(): Flow +} + +/** + * Makes sure the secure flag is set on the activity if the pin is setup. + * @param activity the activity to set the flag on. + */ +fun LockScreenService.handleSecureFlag(activity: ComponentActivity) { + isPinSetup() + .onEach { isPinSetup -> + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { + activity.setRecentsScreenshotEnabled(!isPinSetup) + } else { + if (isPinSetup) { + activity.window.setFlags( + WindowManager.LayoutParams.FLAG_SECURE, + WindowManager.LayoutParams.FLAG_SECURE + ) + } else { + activity.window.clearFlags(WindowManager.LayoutParams.FLAG_SECURE) + } + } + }.launchIn(activity.lifecycleScope) } diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/DefaultLockScreenService.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/DefaultLockScreenService.kt index f4cc699907..96cf7a655b 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/DefaultLockScreenService.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/DefaultLockScreenService.kt @@ -35,8 +35,12 @@ import io.element.android.services.appnavstate.api.AppForegroundStateService import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.delay +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.map import kotlinx.coroutines.launch import javax.inject.Inject import kotlin.time.Duration @@ -113,14 +117,23 @@ class DefaultLockScreenService @Inject constructor( } } - override suspend fun isSetupRequired(): Boolean { - return lockScreenConfig.isPinMandatory - && featureFlagService.isFeatureEnabled(FeatureFlags.PinUnlock) - && !pinCodeManager.isPinCodeAvailable() + override fun isPinSetup(): Flow { + return combine( + featureFlagService.isFeatureEnabledFlow(FeatureFlags.PinUnlock), + pinCodeManager.hasPinCode() + ) { isEnabled, hasPinCode -> + isEnabled && hasPinCode + } + } + + override fun isSetupRequired(): Flow { + return isPinSetup().map { isPinSetup -> + !isPinSetup && lockScreenConfig.isPinMandatory + } } private fun CoroutineScope.lockIfNeeded(gracePeriod: Duration = Duration.ZERO) = launch { - if (featureFlagService.isFeatureEnabled(FeatureFlags.PinUnlock) && pinCodeManager.isPinCodeAvailable()) { + if (isPinSetup().first()) { delay(gracePeriod) _lockScreenState.value = LockScreenLockState.Locked } diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/pin/DefaultPinCodeManager.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/pin/DefaultPinCodeManager.kt index a256562c43..c08513c537 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/pin/DefaultPinCodeManager.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/pin/DefaultPinCodeManager.kt @@ -23,6 +23,7 @@ import io.element.android.libraries.cryptography.api.EncryptionResult import io.element.android.libraries.cryptography.api.SecretKeyRepository import io.element.android.libraries.di.AppScope import io.element.android.libraries.di.SingleIn +import kotlinx.coroutines.flow.Flow import java.util.concurrent.CopyOnWriteArrayList import javax.inject.Inject @@ -46,7 +47,7 @@ class DefaultPinCodeManager @Inject constructor( callbacks.remove(callback) } - override suspend fun isPinCodeAvailable(): Boolean { + override fun hasPinCode(): Flow { return lockScreenStore.hasPinCode() } diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/pin/PinCodeManager.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/pin/PinCodeManager.kt index 21e7281dc8..ae3519759f 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/pin/PinCodeManager.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/pin/PinCodeManager.kt @@ -16,6 +16,8 @@ package io.element.android.features.lockscreen.impl.pin +import kotlinx.coroutines.flow.Flow + /** * This interface is the main interface to manage the pin code. * Implementation should take care of encrypting the pin code and storing it. @@ -55,7 +57,7 @@ interface PinCodeManager { /** * @return true if a pin code is available. */ - suspend fun isPinCodeAvailable(): Boolean + fun hasPinCode(): Flow /** * @return the size of the saved pin code. diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt index 82f5cafbe7..975d049cdb 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt @@ -42,6 +42,7 @@ import io.element.android.libraries.architecture.BackstackNode import io.element.android.libraries.architecture.animation.rememberDefaultTransitionHandler import io.element.android.libraries.architecture.createNode import io.element.android.libraries.di.SessionScope +import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch import kotlinx.parcelize.Parcelize @@ -90,9 +91,11 @@ class LockScreenSettingsFlowNode @AssistedInject constructor( } } - init { + override fun onBuilt() { + super.onBuilt() lifecycleScope.launch { - if (pinCodeManager.isPinCodeAvailable()) { + val hasPinCode = pinCodeManager.hasPinCode().first() + if (hasPinCode) { backstack.newRoot(NavTarget.Unlock) } else { backstack.newRoot(NavTarget.Setup) diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsPresenter.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsPresenter.kt index 2c086ea92a..b60b472f72 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsPresenter.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsPresenter.kt @@ -17,11 +17,10 @@ package io.element.android.features.lockscreen.impl.settings import androidx.compose.runtime.Composable -import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.collectAsState import androidx.compose.runtime.getValue -import androidx.compose.runtime.mutableIntStateOf import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.produceState import androidx.compose.runtime.remember import androidx.compose.runtime.setValue import io.element.android.appconfig.LockScreenConfig @@ -43,23 +42,15 @@ class LockScreenSettingsPresenter @Inject constructor( @Composable override fun present(): LockScreenSettingsState { - var triggerComputation by remember { - mutableIntStateOf(0) - } - var showRemovePinOption by remember { - mutableStateOf(false) - } - var showToggleBiometric by remember { - mutableStateOf(false) + val showRemovePinOption by produceState(initialValue = false) { + pinCodeManager.hasPinCode().collect { hasPinCode -> + value = !lockScreenConfig.isPinMandatory && hasPinCode + } } val isBiometricEnabled by lockScreenStore.isBiometricUnlockAllowed().collectAsState(initial = false) var showRemovePinConfirmation by remember { mutableStateOf(false) } - LaunchedEffect(triggerComputation) { - showRemovePinOption = !lockScreenConfig.isPinMandatory && pinCodeManager.isPinCodeAvailable() - showToggleBiometric = biometricUnlockManager.isDeviceSecured - } fun handleEvents(event: LockScreenSettingsEvents) { when (event) { @@ -69,7 +60,6 @@ class LockScreenSettingsPresenter @Inject constructor( if (showRemovePinConfirmation) { showRemovePinConfirmation = false pinCodeManager.deletePinCode() - triggerComputation++ } } } @@ -86,7 +76,7 @@ class LockScreenSettingsPresenter @Inject constructor( showRemovePinOption = showRemovePinOption, isBiometricEnabled = isBiometricEnabled, showRemovePinConfirmation = showRemovePinConfirmation, - showToggleBiometric = showToggleBiometric, + showToggleBiometric = biometricUnlockManager.isDeviceSecured, eventSink = ::handleEvents ) } diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/EncryptedPinCodeStorage.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/EncryptedPinCodeStorage.kt index 7f19346cec..d2a5ba4204 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/EncryptedPinCodeStorage.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/EncryptedPinCodeStorage.kt @@ -16,6 +16,8 @@ package io.element.android.features.lockscreen.impl.storage +import kotlinx.coroutines.flow.Flow + /** * Should be implemented by any class that provides access to the encrypted PIN code. * All methods are suspending in case there are async IO operations involved. @@ -39,5 +41,6 @@ interface EncryptedPinCodeStorage { /** * Returns whether the PIN code is stored or not. */ - suspend fun hasPinCode(): Boolean + fun hasPinCode(): Flow + } diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/PreferencesLockScreenStore.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/PreferencesLockScreenStore.kt index 4b01b6e62a..cfdf081986 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/PreferencesLockScreenStore.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/storage/PreferencesLockScreenStore.kt @@ -85,10 +85,10 @@ class PreferencesLockScreenStore @Inject constructor( } } - override suspend fun hasPinCode(): Boolean { + override fun hasPinCode(): Flow { return context.dataStore.data.map { preferences -> preferences[pinCodeKey] != null - }.first() + } } override fun isBiometricUnlockAllowed(): Flow { diff --git a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/pin/DefaultPinCodeManagerTest.kt b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/pin/DefaultPinCodeManagerTest.kt index 3c65620084..d3af36d820 100644 --- a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/pin/DefaultPinCodeManagerTest.kt +++ b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/pin/DefaultPinCodeManagerTest.kt @@ -16,6 +16,7 @@ package io.element.android.features.lockscreen.impl.pin +import app.cash.turbine.test import com.google.common.truth.Truth.assertThat import io.element.android.features.lockscreen.impl.pin.storage.InMemoryLockScreenStore import io.element.android.libraries.cryptography.impl.AESEncryptionDecryptionService @@ -32,10 +33,13 @@ class DefaultPinCodeManagerTest { @Test fun `given a pin code when create and delete assert no pin code left`() = runTest { - pinCodeManager.createPinCode("1234") - assertThat(pinCodeManager.isPinCodeAvailable()).isTrue() - pinCodeManager.deletePinCode() - assertThat(pinCodeManager.isPinCodeAvailable()).isFalse() + pinCodeManager.hasPinCode().test { + assertThat(awaitItem()).isFalse() + pinCodeManager.createPinCode("1234") + assertThat(awaitItem()).isTrue() + pinCodeManager.deletePinCode() + assertThat(awaitItem()).isFalse() + } } @Test diff --git a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/pin/storage/InMemoryLockScreenStore.kt b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/pin/storage/InMemoryLockScreenStore.kt index 5d1af46ae5..8f622acfe4 100644 --- a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/pin/storage/InMemoryLockScreenStore.kt +++ b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/pin/storage/InMemoryLockScreenStore.kt @@ -24,7 +24,12 @@ private const val DEFAULT_REMAINING_ATTEMPTS = 3 class InMemoryLockScreenStore : LockScreenStore { + private val hasPinCode = MutableStateFlow(false) private var pinCode: String? = null + set(value) { + field = value + hasPinCode.value = value != null + } private var remainingAttempts: Int = DEFAULT_REMAINING_ATTEMPTS private var isBiometricUnlockAllowed = MutableStateFlow(false) @@ -52,8 +57,8 @@ class InMemoryLockScreenStore : LockScreenStore { pinCode = null } - override suspend fun hasPinCode(): Boolean { - return pinCode != null + override fun hasPinCode(): Flow { + return hasPinCode } override fun isBiometricUnlockAllowed(): Flow { diff --git a/features/lockscreen/test/src/main/kotlin/io/element/android/features/lockscreen/test/FakeLockScreenService.kt b/features/lockscreen/test/src/main/kotlin/io/element/android/features/lockscreen/test/FakeLockScreenService.kt index 012c8e9a5c..11dc4ee20f 100644 --- a/features/lockscreen/test/src/main/kotlin/io/element/android/features/lockscreen/test/FakeLockScreenService.kt +++ b/features/lockscreen/test/src/main/kotlin/io/element/android/features/lockscreen/test/FakeLockScreenService.kt @@ -18,21 +18,27 @@ package io.element.android.features.lockscreen.test import io.element.android.features.lockscreen.api.LockScreenLockState import io.element.android.features.lockscreen.api.LockScreenService +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.map class FakeLockScreenService : LockScreenService { - private var isSetupRequired: Boolean = false + private var isPinSetup = MutableStateFlow(false) private val _lockState: MutableStateFlow = MutableStateFlow(LockScreenLockState.Locked) override val lockState: StateFlow = _lockState - override suspend fun isSetupRequired(): Boolean { - return isSetupRequired + override fun isSetupRequired(): Flow { + return isPinSetup.map { !it } } - fun setIsSetupRequired(isSetupRequired: Boolean) { - this.isSetupRequired = isSetupRequired + fun setIsPinSetup(isPinSetup: Boolean) { + this.isPinSetup.value = isPinSetup + } + + override fun isPinSetup(): Flow { + return isPinSetup } fun setLockState(lockState: LockScreenLockState) { From 43d4f96a9fbc23cc18667205d7376c22cbc3c314 Mon Sep 17 00:00:00 2001 From: ganfra Date: Tue, 7 Nov 2023 16:16:29 +0100 Subject: [PATCH 2/9] LockScreen : do not show the entire setup flow when changing the pin is settings --- .../lockscreen/api/LockScreenService.kt | 3 ++- .../settings/LockScreenSettingsFlowNode.kt | 22 +++++++++---------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/features/lockscreen/api/src/main/kotlin/io/element/android/features/lockscreen/api/LockScreenService.kt b/features/lockscreen/api/src/main/kotlin/io/element/android/features/lockscreen/api/LockScreenService.kt index e0fe432ede..2a628585a1 100644 --- a/features/lockscreen/api/src/main/kotlin/io/element/android/features/lockscreen/api/LockScreenService.kt +++ b/features/lockscreen/api/src/main/kotlin/io/element/android/features/lockscreen/api/LockScreenService.kt @@ -63,5 +63,6 @@ fun LockScreenService.handleSecureFlag(activity: ComponentActivity) { activity.window.clearFlags(WindowManager.LayoutParams.FLAG_SECURE) } } - }.launchIn(activity.lifecycleScope) + } + .launchIn(activity.lifecycleScope) } diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt index 975d049cdb..a052ba65a2 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt @@ -36,7 +36,7 @@ import io.element.android.features.lockscreen.impl.biometric.BiometricUnlockMana import io.element.android.features.lockscreen.impl.biometric.DefaultBiometricUnlockCallback import io.element.android.features.lockscreen.impl.pin.DefaultPinCodeManagerCallback import io.element.android.features.lockscreen.impl.pin.PinCodeManager -import io.element.android.features.lockscreen.impl.setup.LockScreenSetupFlowNode +import io.element.android.features.lockscreen.impl.setup.pin.SetupPinNode import io.element.android.features.lockscreen.impl.unlock.PinUnlockNode import io.element.android.libraries.architecture.BackstackNode import io.element.android.libraries.architecture.animation.rememberDefaultTransitionHandler @@ -69,7 +69,7 @@ class LockScreenSettingsFlowNode @AssistedInject constructor( data object Unlock : NavTarget @Parcelize - data object Setup : NavTarget + data object SetupPin : NavTarget @Parcelize data object Settings : NavTarget @@ -83,6 +83,10 @@ class LockScreenSettingsFlowNode @AssistedInject constructor( override fun onPinCodeRemoved() { navigateUp() } + + override fun onPinCodeCreated() { + backstack.newRoot(NavTarget.Settings) + } } private val biometricUnlockCallback = object : DefaultBiometricUnlockCallback() { @@ -98,7 +102,7 @@ class LockScreenSettingsFlowNode @AssistedInject constructor( if (hasPinCode) { backstack.newRoot(NavTarget.Unlock) } else { - backstack.newRoot(NavTarget.Setup) + backstack.newRoot(NavTarget.SetupPin) } } lifecycle.subscribe( @@ -119,23 +123,19 @@ class LockScreenSettingsFlowNode @AssistedInject constructor( val inputs = PinUnlockNode.Inputs(isInAppUnlock = true) createNode(buildContext, plugins = listOf(inputs)) } - NavTarget.Setup -> { - val callback = object : LockScreenSetupFlowNode.Callback { - override fun onSetupDone() { - backstack.newRoot(NavTarget.Settings) - } - } - createNode(buildContext, plugins = listOf(callback)) + NavTarget.SetupPin -> { + createNode(buildContext) } NavTarget.Settings -> { val callback = object : LockScreenSettingsNode.Callback { override fun onChangePinClicked() { - backstack.push(NavTarget.Setup) + backstack.push(NavTarget.SetupPin) } } createNode(buildContext, plugins = listOf(callback)) } NavTarget.Unknown -> node(buildContext) { } + } } From a5e098dd1f5ad1a82296ed252d05876eba41abb5 Mon Sep 17 00:00:00 2001 From: ganfra Date: Tue, 7 Nov 2023 16:28:16 +0100 Subject: [PATCH 3/9] LockScreen : reduce delay before validation step in setup --- .../lockscreen/impl/setup/pin/SetupPinPresenter.kt | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinPresenter.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinPresenter.kt index 33bf5ccba2..c5f170404f 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinPresenter.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinPresenter.kt @@ -32,6 +32,11 @@ import io.element.android.libraries.core.meta.BuildMeta import kotlinx.coroutines.delay import javax.inject.Inject +/** + * Some time for the ui to refresh before showing confirmation step. + */ +private const val DELAY_BEFORE_CONFIRMATION_STEP_IN_MILLIS = 100L + class SetupPinPresenter @Inject constructor( private val lockScreenConfig: LockScreenConfig, private val pinValidator: PinValidator, @@ -60,8 +65,7 @@ class SetupPinPresenter @Inject constructor( setupPinFailure = pinValidationResult.failure } PinValidator.Result.Valid -> { - // Leave some time for the ui to refresh before showing confirmation - delay(150) + delay(DELAY_BEFORE_CONFIRMATION_STEP_IN_MILLIS) isConfirmationStep = true } } From 41517614c8e3f1b756edac11f6cbe156df9b6af1 Mon Sep 17 00:00:00 2001 From: ganfra Date: Tue, 7 Nov 2023 18:22:40 +0100 Subject: [PATCH 4/9] LockScreen : fix one more navigation issue --- .../features/ftue/impl/FtueFlowNode.kt | 2 +- .../lockscreen/api/LockScreenEntryPoint.kt | 4 +-- .../lockscreen/impl/LockScreenFlowNode.kt | 27 ++++----------- .../impl/biometric/BiometricUnlock.kt | 9 +++-- .../settings/LockScreenSettingsFlowNode.kt | 19 ++++------- .../lockscreen/impl/unlock/PinUnlockNode.kt | 17 ++++++++++ .../impl/unlock/PinUnlockPresenter.kt | 34 +++++++++++++++++-- .../lockscreen/impl/unlock/PinUnlockState.kt | 1 + .../impl/unlock/PinUnlockStateProvider.kt | 2 ++ 9 files changed, 73 insertions(+), 42 deletions(-) diff --git a/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/FtueFlowNode.kt b/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/FtueFlowNode.kt index ab5d163e60..0b77d5e176 100644 --- a/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/FtueFlowNode.kt +++ b/features/ftue/impl/src/main/kotlin/io/element/android/features/ftue/impl/FtueFlowNode.kt @@ -146,7 +146,7 @@ class FtueFlowNode @AssistedInject constructor( } NavTarget.LockScreenSetup -> { val callback = object : LockScreenEntryPoint.Callback { - override fun onSetupCompleted() { + override fun onSetupDone() { lifecycleScope.launch { moveToNextStep() } } } diff --git a/features/lockscreen/api/src/main/kotlin/io/element/android/features/lockscreen/api/LockScreenEntryPoint.kt b/features/lockscreen/api/src/main/kotlin/io/element/android/features/lockscreen/api/LockScreenEntryPoint.kt index f63757717e..fb96895721 100644 --- a/features/lockscreen/api/src/main/kotlin/io/element/android/features/lockscreen/api/LockScreenEntryPoint.kt +++ b/features/lockscreen/api/src/main/kotlin/io/element/android/features/lockscreen/api/LockScreenEntryPoint.kt @@ -31,8 +31,8 @@ interface LockScreenEntryPoint : FeatureEntryPoint { fun build(): Node } - interface Callback: Plugin { - fun onSetupCompleted() + interface Callback : Plugin { + fun onSetupDone() } enum class Target { diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/LockScreenFlowNode.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/LockScreenFlowNode.kt index 48bfa465ee..4818a51bdc 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/LockScreenFlowNode.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/LockScreenFlowNode.kt @@ -20,7 +20,6 @@ import android.os.Parcelable import androidx.compose.runtime.Composable import androidx.compose.ui.Modifier import com.bumble.appyx.core.composable.Children -import com.bumble.appyx.core.lifecycle.subscribe import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -30,8 +29,6 @@ import dagger.assisted.Assisted import dagger.assisted.AssistedInject import io.element.android.anvilannotations.ContributesNode import io.element.android.features.lockscreen.api.LockScreenEntryPoint -import io.element.android.features.lockscreen.impl.pin.DefaultPinCodeManagerCallback -import io.element.android.features.lockscreen.impl.pin.PinCodeManager import io.element.android.features.lockscreen.impl.settings.LockScreenSettingsFlowNode import io.element.android.features.lockscreen.impl.setup.LockScreenSetupFlowNode import io.element.android.features.lockscreen.impl.unlock.PinUnlockNode @@ -46,7 +43,6 @@ import kotlinx.parcelize.Parcelize class LockScreenFlowNode @AssistedInject constructor( @Assisted buildContext: BuildContext, @Assisted plugins: List, - private val pinCodeManager: PinCodeManager, ) : BackstackNode( backstack = BackStack( initialElement = plugins.filterIsInstance(Inputs::class.java).first().initialNavTarget, @@ -71,26 +67,14 @@ class LockScreenFlowNode @AssistedInject constructor( data object Settings : NavTarget } - private val pinCodeManagerCallback = object : DefaultPinCodeManagerCallback() { - override fun onPinCodeCreated() { - plugins().forEach { - it.onSetupCompleted() + private class OnSetupDoneCallback(private val plugins: List) : LockScreenSetupFlowNode.Callback { + override fun onSetupDone() { + plugins.forEach { + it.onSetupDone() } } } - override fun onBuilt() { - super.onBuilt() - lifecycle.subscribe( - onCreate = { - pinCodeManager.addCallback(pinCodeManagerCallback) - }, - onDestroy = { - pinCodeManager.removeCallback(pinCodeManagerCallback) - } - ) - } - override fun resolve(navTarget: NavTarget, buildContext: BuildContext): Node { return when (navTarget) { NavTarget.Unlock -> { @@ -98,7 +82,8 @@ class LockScreenFlowNode @AssistedInject constructor( createNode(buildContext, plugins = listOf(inputs)) } NavTarget.Setup -> { - createNode(buildContext) + val callback = OnSetupDoneCallback(plugins()) + createNode(buildContext, plugins = listOf(callback)) } NavTarget.Settings -> { createNode(buildContext) diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/biometric/BiometricUnlock.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/biometric/BiometricUnlock.kt index e21b8e235c..c7029421e8 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/biometric/BiometricUnlock.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/biometric/BiometricUnlock.kt @@ -24,6 +24,7 @@ import androidx.core.content.ContextCompat import androidx.fragment.app.FragmentActivity import io.element.android.libraries.cryptography.api.EncryptionDecryptionService import io.element.android.libraries.cryptography.api.SecretKeyRepository +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CompletableDeferred import timber.log.Timber import java.security.InvalidKeyException @@ -86,7 +87,12 @@ class DefaultBiometricUnlock( val callback = AuthenticationCallback(callbacks, deferredAuthenticationResult) val prompt = BiometricPrompt(activity, executor, callback) prompt.authenticate(promptInfo, cryptoObject) - return deferredAuthenticationResult.await() + return try { + deferredAuthenticationResult.await() + } catch (cancellation: CancellationException) { + prompt.cancelAuthentication() + BiometricUnlock.AuthenticationResult.Failure(cancellation) + } } @Throws(KeyPermanentlyInvalidatedException::class) @@ -110,7 +116,6 @@ private class AuthenticationCallback( override fun onAuthenticationFailed() { super.onAuthenticationFailed() callbacks.forEach { it.onBiometricUnlockFailed(null) } - deferredAuthenticationResult.complete(BiometricUnlock.AuthenticationResult.Failure(null)) } override fun onAuthenticationSucceeded(result: BiometricPrompt.AuthenticationResult) { diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt index a052ba65a2..ac87529e93 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt @@ -33,7 +33,6 @@ import dagger.assisted.Assisted import dagger.assisted.AssistedInject import io.element.android.anvilannotations.ContributesNode import io.element.android.features.lockscreen.impl.biometric.BiometricUnlockManager -import io.element.android.features.lockscreen.impl.biometric.DefaultBiometricUnlockCallback import io.element.android.features.lockscreen.impl.pin.DefaultPinCodeManagerCallback import io.element.android.features.lockscreen.impl.pin.PinCodeManager import io.element.android.features.lockscreen.impl.setup.pin.SetupPinNode @@ -76,9 +75,6 @@ class LockScreenSettingsFlowNode @AssistedInject constructor( } private val pinCodeManagerCallback = object : DefaultPinCodeManagerCallback() { - override fun onPinCodeVerified() { - backstack.newRoot(NavTarget.Settings) - } override fun onPinCodeRemoved() { navigateUp() @@ -89,12 +85,6 @@ class LockScreenSettingsFlowNode @AssistedInject constructor( } } - private val biometricUnlockCallback = object : DefaultBiometricUnlockCallback() { - override fun onBiometricUnlockSuccess() { - backstack.newRoot(NavTarget.Settings) - } - } - override fun onBuilt() { super.onBuilt() lifecycleScope.launch { @@ -108,11 +98,9 @@ class LockScreenSettingsFlowNode @AssistedInject constructor( lifecycle.subscribe( onCreate = { pinCodeManager.addCallback(pinCodeManagerCallback) - biometricUnlockManager.addCallback(biometricUnlockCallback) }, onDestroy = { pinCodeManager.removeCallback(pinCodeManagerCallback) - biometricUnlockManager.removeCallback(biometricUnlockCallback) } ) } @@ -121,7 +109,12 @@ class LockScreenSettingsFlowNode @AssistedInject constructor( return when (navTarget) { NavTarget.Unlock -> { val inputs = PinUnlockNode.Inputs(isInAppUnlock = true) - createNode(buildContext, plugins = listOf(inputs)) + val callback = object : PinUnlockNode.Callback { + override fun onUnlock() { + backstack.newRoot(NavTarget.Settings) + } + } + createNode(buildContext, plugins = listOf(inputs, callback)) } NavTarget.SetupPin -> { createNode(buildContext) diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockNode.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockNode.kt index a0818716b6..fbda73fd45 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockNode.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockNode.kt @@ -17,10 +17,12 @@ package io.element.android.features.lockscreen.impl.unlock import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect import androidx.compose.ui.Modifier import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin +import com.bumble.appyx.core.plugin.plugins import dagger.assisted.Assisted import dagger.assisted.AssistedInject import io.element.android.anvilannotations.ContributesNode @@ -35,15 +37,30 @@ class PinUnlockNode @AssistedInject constructor( private val presenter: PinUnlockPresenter, ) : Node(buildContext, plugins = plugins) { + interface Callback : Plugin { + fun onUnlock() + } + data class Inputs( val isInAppUnlock: Boolean ) : NodeInputs private val inputs: Inputs = inputs() + private fun onUnlock() { + plugins().forEach { + it.onUnlock() + } + } + @Composable override fun View(modifier: Modifier) { val state = presenter.present() + LaunchedEffect(state.isUnlocked) { + if (state.isUnlocked) { + onUnlock() + } + } PinUnlockView( state = state, isInAppUnlock = inputs.isInAppUnlock, diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenter.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenter.kt index 872547dfdc..21f08a3829 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenter.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenter.kt @@ -17,6 +17,7 @@ package io.element.android.features.lockscreen.impl.unlock import androidx.compose.runtime.Composable +import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.MutableState import androidx.compose.runtime.getValue @@ -26,6 +27,8 @@ import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import io.element.android.features.lockscreen.impl.biometric.BiometricUnlock import io.element.android.features.lockscreen.impl.biometric.BiometricUnlockManager +import io.element.android.features.lockscreen.impl.biometric.DefaultBiometricUnlockCallback +import io.element.android.features.lockscreen.impl.pin.DefaultPinCodeManagerCallback import io.element.android.features.lockscreen.impl.pin.PinCodeManager import io.element.android.features.lockscreen.impl.pin.model.PinEntry import io.element.android.features.lockscreen.impl.unlock.keypad.PinKeypadModel @@ -36,6 +39,7 @@ import io.element.android.libraries.core.bool.orFalse import io.element.android.libraries.matrix.api.MatrixClient import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.launch +import timber.log.Timber import javax.inject.Inject class PinUnlockPresenter @Inject constructor( @@ -66,9 +70,10 @@ class PinUnlockPresenter @Inject constructor( var biometricUnlockResult by remember { mutableStateOf(null) } - + val isUnlocked = remember { + mutableStateOf(false) + } val biometricUnlock = biometricUnlockManager.rememberBiometricUnlock() - LaunchedEffect(Unit) { suspend { val pinCodeSize = pinCodeManager.getPinCodeSize() @@ -94,7 +99,7 @@ class PinUnlockPresenter @Inject constructor( showSignOutPrompt = true } } - + IsUnlockedEffect(isUnlocked) fun handleEvents(event: PinUnlockEvents) { when (event) { is PinUnlockEvents.OnPinKeypadPressed -> { @@ -129,10 +134,33 @@ class PinUnlockPresenter @Inject constructor( signOutAction = signOutAction.value, showBiometricUnlock = biometricUnlock.isActive, biometricUnlockResult = biometricUnlockResult, + isUnlocked = isUnlocked.value, eventSink = ::handleEvents ) } + @Composable + private fun IsUnlockedEffect(isUnlocked: MutableState) { + DisposableEffect(Unit) { + val biometricUnlockCallback = object : DefaultBiometricUnlockCallback() { + override fun onBiometricUnlockSuccess() { + isUnlocked.value = true + } + } + val pinCodeVerifiedCallback = object : DefaultPinCodeManagerCallback() { + override fun onPinCodeVerified() { + isUnlocked.value = true + } + } + biometricUnlockManager.addCallback(biometricUnlockCallback) + pinCodeManager.addCallback(pinCodeVerifiedCallback) + onDispose { + biometricUnlockManager.removeCallback(biometricUnlockCallback) + pinCodeManager.removeCallback(pinCodeVerifiedCallback) + } + } + } + private fun Async.isComplete(): Boolean { return dataOrNull()?.isComplete().orFalse() } diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockState.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockState.kt index 667f7a825a..006234270c 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockState.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockState.kt @@ -28,6 +28,7 @@ data class PinUnlockState( val showSignOutPrompt: Boolean, val signOutAction: Async, val showBiometricUnlock: Boolean, + val isUnlocked: Boolean, val biometricUnlockResult: BiometricUnlock.AuthenticationResult?, val eventSink: (PinUnlockEvents) -> Unit ) { diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockStateProvider.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockStateProvider.kt index cbf6b2dee2..02da0e0350 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockStateProvider.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockStateProvider.kt @@ -41,6 +41,7 @@ fun aPinUnlockState( showSignOutPrompt: Boolean = false, showBiometricUnlock: Boolean = true, biometricUnlockResult: BiometricUnlock.AuthenticationResult? = null, + isUnlocked: Boolean = false, signOutAction: Async = Async.Uninitialized, ) = PinUnlockState( pinEntry = Async.Success(pinEntry), @@ -50,5 +51,6 @@ fun aPinUnlockState( showBiometricUnlock = showBiometricUnlock, signOutAction = signOutAction, biometricUnlockResult = biometricUnlockResult, + isUnlocked = isUnlocked, eventSink = {} ) From 8b4d3a4bc80f1cf9f3213202ff2e2bbe524c95f4 Mon Sep 17 00:00:00 2001 From: ganfra Date: Tue, 7 Nov 2023 21:06:26 +0100 Subject: [PATCH 5/9] Lock : fix race condition on setup pin --- .../lockscreen/impl/setup/pin/SetupPinEvents.kt | 2 +- .../impl/setup/pin/SetupPinPresenter.kt | 3 ++- .../lockscreen/impl/setup/pin/SetupPinView.kt | 4 ++-- .../impl/setup/pin/SetupPinPresenterTest.kt | 16 ++++++++++------ 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinEvents.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinEvents.kt index d0105b1a21..15de84c863 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinEvents.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinEvents.kt @@ -17,6 +17,6 @@ package io.element.android.features.lockscreen.impl.setup.pin sealed interface SetupPinEvents { - data class OnPinEntryChanged(val entryAsText: String) : SetupPinEvents + data class OnPinEntryChanged(val entryAsText: String, val fromConfirmationStep: Boolean) : SetupPinEvents data object ClearFailure : SetupPinEvents } diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinPresenter.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinPresenter.kt index c5f170404f..d48e418a59 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinPresenter.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinPresenter.kt @@ -85,7 +85,8 @@ class SetupPinPresenter @Inject constructor( fun handleEvents(event: SetupPinEvents) { when (event) { is SetupPinEvents.OnPinEntryChanged -> { - if (isConfirmationStep) { + // Use the fromConfirmationStep flag from ui to avoid race condition. + if (event.fromConfirmationStep) { confirmPinEntry = confirmPinEntry.fillWith(event.entryAsText) } else { choosePinEntry = choosePinEntry.fillWith(event.entryAsText) diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinView.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinView.kt index 3d41d1ba1d..095848c9fb 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinView.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinView.kt @@ -116,8 +116,8 @@ private fun SetupPinContent( PinEntryTextField( pinEntry = state.activePinEntry, isSecured = true, - onValueChange = { - state.eventSink(SetupPinEvents.OnPinEntryChanged(it)) + onValueChange = { entry -> + state.eventSink(SetupPinEvents.OnPinEntryChanged(entry, state.isConfirmationStep)) }, modifier = modifier .focusRequester(focusRequester) diff --git a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinPresenterTest.kt b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinPresenterTest.kt index 7f3373bd4a..7ec73116c2 100644 --- a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinPresenterTest.kt +++ b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/setup/pin/SetupPinPresenterTest.kt @@ -60,14 +60,14 @@ class SetupPinPresenterTest { state.confirmPinEntry.assertEmpty() assertThat(state.setupPinFailure).isNull() assertThat(state.isConfirmationStep).isFalse() - state.eventSink(SetupPinEvents.OnPinEntryChanged(halfCompletePin)) + state.onPinEntryChanged(halfCompletePin) } awaitItem().also { state -> state.choosePinEntry.assertText(halfCompletePin) state.confirmPinEntry.assertEmpty() assertThat(state.setupPinFailure).isNull() assertThat(state.isConfirmationStep).isFalse() - state.eventSink(SetupPinEvents.OnPinEntryChanged(blacklistedPin)) + state.onPinEntryChanged(blacklistedPin) } awaitLastSequentialItem().also { state -> state.choosePinEntry.assertText(blacklistedPin) @@ -77,7 +77,7 @@ class SetupPinPresenterTest { awaitLastSequentialItem().also { state -> state.choosePinEntry.assertEmpty() assertThat(state.setupPinFailure).isNull() - state.eventSink(SetupPinEvents.OnPinEntryChanged(completePin)) + state.onPinEntryChanged(completePin) } consumeItemsUntilPredicate { it.isConfirmationStep @@ -85,7 +85,7 @@ class SetupPinPresenterTest { state.choosePinEntry.assertText(completePin) state.confirmPinEntry.assertEmpty() assertThat(state.isConfirmationStep).isTrue() - state.eventSink(SetupPinEvents.OnPinEntryChanged(mismatchedPin)) + state.onPinEntryChanged(mismatchedPin) } awaitLastSequentialItem().also { state -> state.choosePinEntry.assertText(completePin) @@ -98,7 +98,7 @@ class SetupPinPresenterTest { state.confirmPinEntry.assertEmpty() assertThat(state.isConfirmationStep).isFalse() assertThat(state.setupPinFailure).isNull() - state.eventSink(SetupPinEvents.OnPinEntryChanged(completePin)) + state.onPinEntryChanged(completePin) } consumeItemsUntilPredicate { it.isConfirmationStep @@ -106,7 +106,7 @@ class SetupPinPresenterTest { state.choosePinEntry.assertText(completePin) state.confirmPinEntry.assertEmpty() assertThat(state.isConfirmationStep).isTrue() - state.eventSink(SetupPinEvents.OnPinEntryChanged(completePin)) + state.onPinEntryChanged(completePin) } awaitItem().also { state -> state.choosePinEntry.assertText(completePin) @@ -116,6 +116,10 @@ class SetupPinPresenterTest { } } + private fun SetupPinState.onPinEntryChanged(pinEntry: String){ + eventSink(SetupPinEvents.OnPinEntryChanged(pinEntry, isConfirmationStep)) + } + private fun createSetupPinPresenter( callback: PinCodeManager.Callback, lockScreenConfig: LockScreenConfig = aLockScreenConfig( From 39da7e75a58c6d36bbef86a7aff588a4d73b1a6e Mon Sep 17 00:00:00 2001 From: ganfra Date: Tue, 7 Nov 2023 21:08:02 +0100 Subject: [PATCH 6/9] Lock screen : fix code quality --- .../impl/settings/LockScreenSettingsFlowNode.kt | 2 -- .../lockscreen/impl/unlock/PinUnlockPresenter.kt | 13 ++++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt index ac87529e93..dec3699d07 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/settings/LockScreenSettingsFlowNode.kt @@ -32,7 +32,6 @@ import com.bumble.appyx.navmodel.backstack.operation.push import dagger.assisted.Assisted import dagger.assisted.AssistedInject import io.element.android.anvilannotations.ContributesNode -import io.element.android.features.lockscreen.impl.biometric.BiometricUnlockManager import io.element.android.features.lockscreen.impl.pin.DefaultPinCodeManagerCallback import io.element.android.features.lockscreen.impl.pin.PinCodeManager import io.element.android.features.lockscreen.impl.setup.pin.SetupPinNode @@ -50,7 +49,6 @@ class LockScreenSettingsFlowNode @AssistedInject constructor( @Assisted buildContext: BuildContext, @Assisted plugins: List, private val pinCodeManager: PinCodeManager, - private val biometricUnlockManager: BiometricUnlockManager, ) : BackstackNode( backstack = BackStack( initialElement = NavTarget.Unknown, diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenter.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenter.kt index 21f08a3829..b2d1135963 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenter.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenter.kt @@ -39,7 +39,6 @@ import io.element.android.libraries.core.bool.orFalse import io.element.android.libraries.matrix.api.MatrixClient import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.launch -import timber.log.Timber import javax.inject.Inject class PinUnlockPresenter @Inject constructor( @@ -99,7 +98,11 @@ class PinUnlockPresenter @Inject constructor( showSignOutPrompt = true } } - IsUnlockedEffect(isUnlocked) + + OnUnlockEffect { + isUnlocked.value = true + } + fun handleEvents(event: PinUnlockEvents) { when (event) { is PinUnlockEvents.OnPinKeypadPressed -> { @@ -140,16 +143,16 @@ class PinUnlockPresenter @Inject constructor( } @Composable - private fun IsUnlockedEffect(isUnlocked: MutableState) { + private fun OnUnlockEffect(onUnlock: () -> Unit) { DisposableEffect(Unit) { val biometricUnlockCallback = object : DefaultBiometricUnlockCallback() { override fun onBiometricUnlockSuccess() { - isUnlocked.value = true + onUnlock() } } val pinCodeVerifiedCallback = object : DefaultPinCodeManagerCallback() { override fun onPinCodeVerified() { - isUnlocked.value = true + onUnlock() } } biometricUnlockManager.addCallback(biometricUnlockCallback) From dc2e05430b05805208492bb4305448179a72e569 Mon Sep 17 00:00:00 2001 From: ganfra Date: Wed, 8 Nov 2023 11:53:01 +0100 Subject: [PATCH 7/9] Lockscreen : should fix tests --- .../lockscreen/impl/unlock/PinUnlockHelper.kt | 53 +++++++++++++++++++ .../impl/unlock/PinUnlockPresenter.kt | 29 +--------- .../impl/unlock/PinUnlockPresenterTest.kt | 20 ++----- 3 files changed, 59 insertions(+), 43 deletions(-) create mode 100644 features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockHelper.kt diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockHelper.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockHelper.kt new file mode 100644 index 0000000000..72d29b4481 --- /dev/null +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockHelper.kt @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.features.lockscreen.impl.unlock + +import androidx.compose.runtime.Composable +import androidx.compose.runtime.DisposableEffect +import io.element.android.features.lockscreen.impl.biometric.BiometricUnlockManager +import io.element.android.features.lockscreen.impl.biometric.DefaultBiometricUnlockCallback +import io.element.android.features.lockscreen.impl.pin.DefaultPinCodeManagerCallback +import io.element.android.features.lockscreen.impl.pin.PinCodeManager +import javax.inject.Inject + +class PinUnlockHelper @Inject constructor( + private val biometricUnlockManager: BiometricUnlockManager, + private val pinCodeManager: PinCodeManager +) { + + @Composable + fun OnUnlockEffect(onUnlock: () -> Unit) { + DisposableEffect(Unit) { + val biometricUnlockCallback = object : DefaultBiometricUnlockCallback() { + override fun onBiometricUnlockSuccess() { + onUnlock() + } + } + val pinCodeVerifiedCallback = object : DefaultPinCodeManagerCallback() { + override fun onPinCodeVerified() { + onUnlock() + } + } + biometricUnlockManager.addCallback(biometricUnlockCallback) + pinCodeManager.addCallback(pinCodeVerifiedCallback) + onDispose { + biometricUnlockManager.removeCallback(biometricUnlockCallback) + pinCodeManager.removeCallback(pinCodeVerifiedCallback) + } + } + } +} diff --git a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenter.kt b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenter.kt index b2d1135963..1b5ce80085 100644 --- a/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenter.kt +++ b/features/lockscreen/impl/src/main/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenter.kt @@ -17,7 +17,6 @@ package io.element.android.features.lockscreen.impl.unlock import androidx.compose.runtime.Composable -import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.MutableState import androidx.compose.runtime.getValue @@ -27,8 +26,6 @@ import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import io.element.android.features.lockscreen.impl.biometric.BiometricUnlock import io.element.android.features.lockscreen.impl.biometric.BiometricUnlockManager -import io.element.android.features.lockscreen.impl.biometric.DefaultBiometricUnlockCallback -import io.element.android.features.lockscreen.impl.pin.DefaultPinCodeManagerCallback import io.element.android.features.lockscreen.impl.pin.PinCodeManager import io.element.android.features.lockscreen.impl.pin.model.PinEntry import io.element.android.features.lockscreen.impl.unlock.keypad.PinKeypadModel @@ -46,6 +43,7 @@ class PinUnlockPresenter @Inject constructor( private val biometricUnlockManager: BiometricUnlockManager, private val matrixClient: MatrixClient, private val coroutineScope: CoroutineScope, + private val pinUnlockHelper: PinUnlockHelper, ) : Presenter { @Composable @@ -98,8 +96,7 @@ class PinUnlockPresenter @Inject constructor( showSignOutPrompt = true } } - - OnUnlockEffect { + pinUnlockHelper.OnUnlockEffect { isUnlocked.value = true } @@ -142,28 +139,6 @@ class PinUnlockPresenter @Inject constructor( ) } - @Composable - private fun OnUnlockEffect(onUnlock: () -> Unit) { - DisposableEffect(Unit) { - val biometricUnlockCallback = object : DefaultBiometricUnlockCallback() { - override fun onBiometricUnlockSuccess() { - onUnlock() - } - } - val pinCodeVerifiedCallback = object : DefaultPinCodeManagerCallback() { - override fun onPinCodeVerified() { - onUnlock() - } - } - biometricUnlockManager.addCallback(biometricUnlockCallback) - pinCodeManager.addCallback(pinCodeVerifiedCallback) - onDispose { - biometricUnlockManager.removeCallback(biometricUnlockCallback) - pinCodeManager.removeCallback(pinCodeVerifiedCallback) - } - } - } - private fun Async.isComplete(): Boolean { return dataOrNull()?.isComplete().orFalse() } diff --git a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenterTest.kt b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenterTest.kt index 95ded7617c..745b1c9629 100644 --- a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenterTest.kt +++ b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenterTest.kt @@ -32,7 +32,6 @@ import io.element.android.libraries.architecture.Async import io.element.android.libraries.matrix.test.FakeMatrixClient import io.element.android.tests.testutils.awaitLastSequentialItem import io.element.android.tests.testutils.consumeItemsUntilPredicate -import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.test.runTest import org.junit.Test @@ -44,13 +43,7 @@ class PinUnlockPresenterTest { @Test fun `present - success verify flow`() = runTest { - val pinCodeVerified = CompletableDeferred() - val callback = object : DefaultPinCodeManagerCallback() { - override fun onPinCodeCreated() { - pinCodeVerified.complete(Unit) - } - } - val presenter = createPinUnlockPresenter(this, callback = callback) + val presenter = createPinUnlockPresenter(this) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -58,6 +51,7 @@ class PinUnlockPresenterTest { assertThat(state.pinEntry).isInstanceOf(Async.Uninitialized::class.java) assertThat(state.showWrongPinTitle).isFalse() assertThat(state.showSignOutPrompt).isFalse() + assertThat(state.isUnlocked).isFalse() assertThat(state.signOutAction).isInstanceOf(Async.Uninitialized::class.java) assertThat(state.remainingAttempts).isInstanceOf(Async.Uninitialized::class.java) } @@ -77,20 +71,14 @@ class PinUnlockPresenterTest { } awaitLastSequentialItem().also { state -> state.pinEntry.assertText(completePin) + assertThat(state.isUnlocked).isTrue() } - pinCodeVerified.await() } } @Test fun `present - failure verify flow`() = runTest { - val pinCodeVerified = CompletableDeferred() - val callback = object : DefaultPinCodeManagerCallback() { - override fun onPinCodeCreated() { - pinCodeVerified.complete(Unit) - } - } - val presenter = createPinUnlockPresenter(this, callback = callback) + val presenter = createPinUnlockPresenter(this) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { From f2ae715a0799d711f0dbb6fefe01bcf57dcdc1a3 Mon Sep 17 00:00:00 2001 From: ganfra Date: Wed, 8 Nov 2023 12:54:25 +0100 Subject: [PATCH 8/9] Update features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueStateTests.kt Co-authored-by: Benoit Marty --- .../element/android/features/ftue/impl/DefaultFtueStateTests.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueStateTests.kt b/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueStateTests.kt index 13de50c6b4..37e61ebf55 100644 --- a/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueStateTests.kt +++ b/features/ftue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueStateTests.kt @@ -113,7 +113,7 @@ class DefaultFtueStateTests { steps.add(state.getNextStep(steps.lastOrNull())) permissionStateProvider.setPermissionGranted() - // Fourth step, notifications opt in + // Fourth step, entering PIN code steps.add(state.getNextStep(steps.lastOrNull())) lockScreenService.setIsPinSetup(true) From a5e23431a7ab0fbf71a88e42b2433c456c8b1c24 Mon Sep 17 00:00:00 2001 From: ganfra Date: Wed, 8 Nov 2023 13:45:28 +0100 Subject: [PATCH 9/9] Lock screen : fix one more test. --- .../features/lockscreen/impl/unlock/PinUnlockPresenterTest.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenterTest.kt b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenterTest.kt index 745b1c9629..021f0132a2 100644 --- a/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenterTest.kt +++ b/features/lockscreen/impl/src/test/kotlin/io/element/android/features/lockscreen/impl/unlock/PinUnlockPresenterTest.kt @@ -148,6 +148,7 @@ class PinUnlockPresenterTest { biometricUnlockManager = biometricUnlockManager, matrixClient = FakeMatrixClient(), coroutineScope = scope, + pinUnlockHelper = PinUnlockHelper(biometricUnlockManager, pinCodeManager), ) } }