Remove SessionData.needsVerification as the source of truth for ses…

…sion verification status (element-hq#2748) * Remove `SessionData.needsVerification` as the source of truth for session verification status. - Use the Rust SDK `EncryptionService.verificationState()` instead, but always waiting for the first 'known' result (either verified or not, discarding 'unknown'). - Add a workaround in the super rare case when reading this value gets stuck somehow. We'll assume the user is not verified in that case. - Make `DefaultFtueService.getNextStep` and dependent checks `suspend`. - Make the `skip` button use a value in the session preferences instead. * Log exception when the verification status can't be loaded Co-authored-by: Benoit Marty <[email protected]> * Fix review comments --------- Co-authored-by: Benoit Marty <[email protected]>
rotekii · Apr 24, 2024 · 1de6797 · 1de6797
1 parent a27afaf
commit 1de6797
Show file tree

Hide file tree

Showing 26 changed files with 99 additions and 106 deletions.
diff --git a/changelog.d/2718.bugfix b/changelog.d/2718.bugfix
@@ -0,0 +1 @@
+Fix session verification being asked again for already verified users.
diff --git a/features/ftue/impl/build.gradle.kts b/features/ftue/impl/build.gradle.kts
@@ -39,6 +39,7 @@ dependencies {
     implementation(projects.libraries.matrix.api)
     implementation(projects.libraries.matrixui)
     implementation(projects.libraries.designsystem)
+    implementation(projects.libraries.preferences.api)
     implementation(projects.libraries.uiStrings)
     implementation(projects.libraries.testtags)
     implementation(projects.features.analytics.api)
@@ -60,6 +61,7 @@ dependencies {
     testImplementation(projects.services.analytics.test)
     testImplementation(projects.libraries.permissions.impl)
     testImplementation(projects.libraries.permissions.test)
+    testImplementation(projects.libraries.preferences.test)
     testImplementation(projects.features.lockscreen.test)
     testImplementation(projects.tests.testutils)
 

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
@@ -143,7 +143,7 @@ class FtueFlowNode @AssistedInject constructor(
         }
     }
 
-    private fun moveToNextStep() {
+    private fun moveToNextStep() = lifecycleScope.launch {
         when (ftueState.getNextStep()) {
             FtueStep.SessionVerification -> {
                 backstack.newRoot(NavTarget.SessionVerification)

diff --git a/...ue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueService.kt b/...ue/impl/src/main/kotlin/io/element/android/features/ftue/impl/state/DefaultFtueService.kt
@@ -23,18 +23,26 @@ import com.squareup.anvil.annotations.ContributesBinding
 import io.element.android.features.ftue.api.state.FtueService
 import io.element.android.features.ftue.api.state.FtueState
 import io.element.android.features.lockscreen.api.LockScreenService
+import io.element.android.features.preferences.api.store.SessionPreferencesStore
 import io.element.android.libraries.di.SessionScope
 import io.element.android.libraries.matrix.api.verification.SessionVerificationService
+import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus
 import io.element.android.libraries.permissions.api.PermissionStateProvider
 import io.element.android.services.analytics.api.AnalyticsService
 import io.element.android.services.toolbox.api.sdk.BuildVersionSdkIntProvider
 import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.FlowPreview
 import kotlinx.coroutines.flow.MutableStateFlow
+import kotlinx.coroutines.flow.catch
+import kotlinx.coroutines.flow.filter
 import kotlinx.coroutines.flow.first
 import kotlinx.coroutines.flow.launchIn
 import kotlinx.coroutines.flow.onEach
+import kotlinx.coroutines.flow.timeout
 import kotlinx.coroutines.runBlocking
+import timber.log.Timber
 import javax.inject.Inject
+import kotlin.time.Duration.Companion.seconds
 
 @ContributesBinding(SessionScope::class)
 class DefaultFtueService @Inject constructor(
@@ -44,6 +52,7 @@ class DefaultFtueService @Inject constructor(
     private val permissionStateProvider: PermissionStateProvider,
     private val lockScreenService: LockScreenService,
     private val sessionVerificationService: SessionVerificationService,
+    private val sessionPreferencesStore: SessionPreferencesStore,
 ) : FtueService {
     override val state = MutableStateFlow<FtueState>(FtueState.Unknown)
 
@@ -55,7 +64,7 @@ class DefaultFtueService @Inject constructor(
     }
 
     init {
-        sessionVerificationService.needsVerificationFlow
+        sessionVerificationService.sessionVerifiedStatus
             .onEach { updateState() }
             .launchIn(coroutineScope)
 
@@ -64,7 +73,7 @@ class DefaultFtueService @Inject constructor(
             .launchIn(coroutineScope)
     }
 
-    fun getNextStep(currentStep: FtueStep? = null): FtueStep? =
+    suspend fun getNextStep(currentStep: FtueStep? = null): FtueStep? =
         when (currentStep) {
             null -> if (isSessionNotVerified()) {
                 FtueStep.SessionVerification
@@ -89,25 +98,37 @@ class DefaultFtueService @Inject constructor(
             FtueStep.AnalyticsOptIn -> null
         }
 
-    private fun isAnyStepIncomplete(): Boolean {
-        return listOf(
+    private suspend fun isAnyStepIncomplete(): Boolean {
+        return listOf<suspend () -> Boolean>(
             { isSessionNotVerified() },
             { shouldAskNotificationPermissions() },
             { needsAnalyticsOptIn() },
             { shouldDisplayLockscreenSetup() },
         ).any { it() }
     }
 
-    private fun isSessionNotVerified(): Boolean {
-        return sessionVerificationService.needsVerificationFlow.value
+    @OptIn(FlowPreview::class)
+    private suspend fun isSessionNotVerified(): Boolean {
+        // Wait for the first known (or ready) verification status
+        val readyVerifiedSessionStatus = sessionVerificationService.sessionVerifiedStatus
+            .filter { it != SessionVerifiedStatus.Unknown }
+            // This is not ideal, but there are some very rare cases when reading the flow seems to get stuck
+            .timeout(5.seconds)
+            .catch {
+                Timber.e(it, "Failed to get session verification status, assume it's not verified")
+                emit(SessionVerifiedStatus.NotVerified)
+            }
+            .first()
+        val skipVerification = suspend { sessionPreferencesStore.isSessionVerificationSkipped().first() }
+        return readyVerifiedSessionStatus == SessionVerifiedStatus.NotVerified && !skipVerification()
     }
 
-    private fun needsAnalyticsOptIn(): Boolean {
+    private suspend fun needsAnalyticsOptIn(): Boolean {
         // We need this function to not be suspend, so we need to load the value through runBlocking
-        return runBlocking { analyticsService.didAskUserConsent().first().not() }
+        return analyticsService.didAskUserConsent().first().not()
     }
 
-    private fun shouldAskNotificationPermissions(): Boolean {
+    private suspend fun shouldAskNotificationPermissions(): Boolean {
         return if (sdkVersionProvider.isAtLeast(Build.VERSION_CODES.TIRAMISU)) {
             val permission = Manifest.permission.POST_NOTIFICATIONS
             val isPermissionDenied = runBlocking { permissionStateProvider.isPermissionDenied(permission).first() }
@@ -118,14 +139,12 @@ class DefaultFtueService @Inject constructor(
         }
     }
 
-    private fun shouldDisplayLockscreenSetup(): Boolean {
-        return runBlocking {
-            lockScreenService.isSetupRequired().first()
-        }
+    private suspend fun shouldDisplayLockscreenSetup(): Boolean {
+        return lockScreenService.isSetupRequired().first()
     }
 
     @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE)
-    internal fun updateState() {
+    internal suspend fun updateState() {
         state.value = when {
             isAnyStepIncomplete() -> FtueState.Incomplete
             else -> FtueState.Complete

diff --git a/...tue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueServiceTests.kt b/...tue/impl/src/test/kotlin/io/element/android/features/ftue/impl/DefaultFtueServiceTests.kt
@@ -24,6 +24,7 @@ import io.element.android.features.ftue.impl.state.DefaultFtueService
 import io.element.android.features.ftue.impl.state.FtueStep
 import io.element.android.features.lockscreen.api.LockScreenService
 import io.element.android.features.lockscreen.test.FakeLockScreenService
+import io.element.android.libraries.featureflag.test.InMemorySessionPreferencesStore
 import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus
 import io.element.android.libraries.matrix.test.verification.FakeSessionVerificationService
 import io.element.android.libraries.permissions.impl.FakePermissionStateProvider
@@ -90,7 +91,6 @@ class DefaultFtueServiceTests {
     fun `traverse flow`() = runTest {
         val sessionVerificationService = FakeSessionVerificationService().apply {
             givenVerifiedStatus(SessionVerifiedStatus.NotVerified)
-            givenNeedsVerification(true)
         }
         val analyticsService = FakeAnalyticsService()
         val permissionStateProvider = FakePermissionStateProvider(permissionGranted = false)
@@ -108,7 +108,7 @@ class DefaultFtueServiceTests {
 
         // Session verification
         steps.add(state.getNextStep(steps.lastOrNull()))
-        sessionVerificationService.givenNeedsVerification(false)
+        sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.NotVerified)
 
         // Notifications opt in
         steps.add(state.getNextStep(steps.lastOrNull()))
@@ -200,6 +200,7 @@ class DefaultFtueServiceTests {
         analyticsService: AnalyticsService = FakeAnalyticsService(),
         permissionStateProvider: FakePermissionStateProvider = FakePermissionStateProvider(permissionGranted = false),
         lockScreenService: LockScreenService = FakeLockScreenService(),
+        sessionPreferencesStore: InMemorySessionPreferencesStore = InMemorySessionPreferencesStore(),
         // First version where notification permission is required
         sdkIntVersion: Int = Build.VERSION_CODES.TIRAMISU,
     ) = DefaultFtueService(
@@ -209,5 +210,6 @@ class DefaultFtueServiceTests {
         analyticsService = analyticsService,
         permissionStateProvider = permissionStateProvider,
         lockScreenService = lockScreenService,
+        sessionPreferencesStore = sessionPreferencesStore,
     )
 }
diff --git a/...impl/src/main/kotlin/io/element/android/features/signedout/impl/SignedOutStateProvider.kt b/...impl/src/main/kotlin/io/element/android/features/signedout/impl/SignedOutStateProvider.kt
@@ -38,7 +38,6 @@ fun aSignedOutState() = SignedOutState(
 fun aSessionData(
     sessionId: SessionId = SessionId("@alice:server.org"),
     isTokenValid: Boolean = false,
-    needsVerification: Boolean = false,
 ): SessionData {
     return SessionData(
         userId = sessionId.value,
@@ -52,6 +51,5 @@ fun aSessionData(
         isTokenValid = isTokenValid,
         loginType = LoginType.UNKNOWN,
         passphrase = null,
-        needsVerification = needsVerification,
     )
 }
diff --git a/features/verifysession/impl/build.gradle.kts b/features/verifysession/impl/build.gradle.kts
@@ -42,6 +42,7 @@ dependencies {
     implementation(projects.libraries.matrix.api)
     implementation(projects.libraries.matrixui)
     implementation(projects.libraries.designsystem)
+    implementation(projects.libraries.preferences.api)
     implementation(projects.libraries.uiStrings)
     api(libs.statemachine)
     api(projects.features.verifysession.api)
@@ -53,6 +54,7 @@ dependencies {
     testImplementation(libs.test.truth)
     testImplementation(libs.test.turbine)
     testImplementation(projects.libraries.matrix.test)
+    testImplementation(projects.libraries.preferences.test)
     testImplementation(projects.tests.testutils)
     testImplementation(libs.androidx.compose.ui.test.junit)
     testReleaseImplementation(libs.androidx.compose.ui.test.manifest)

diff --git a/.../main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionPresenter.kt b/.../main/kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionPresenter.kt
@@ -23,11 +23,11 @@ import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.collectAsState
 import androidx.compose.runtime.derivedStateOf
 import androidx.compose.runtime.getValue
-import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
 import androidx.compose.runtime.rememberCoroutineScope
 import androidx.compose.runtime.setValue
 import com.freeletics.flowredux.compose.rememberStateAndDispatch
+import io.element.android.features.preferences.api.store.SessionPreferencesStore
 import io.element.android.libraries.architecture.AsyncData
 import io.element.android.libraries.architecture.Presenter
 import io.element.android.libraries.core.meta.BuildMeta
@@ -49,6 +49,7 @@ class VerifySelfSessionPresenter @Inject constructor(
     private val encryptionService: EncryptionService,
     private val stateMachine: VerifySelfSessionStateMachine,
     private val buildMeta: BuildMeta,
+    private val sessionPreferencesStore: SessionPreferencesStore,
 ) : Presenter<VerifySelfSessionState> {
     @Composable
     override fun present(): VerifySelfSessionState {
@@ -59,8 +60,8 @@ class VerifySelfSessionPresenter @Inject constructor(
         }
         val recoveryState by encryptionService.recoveryStateStateFlow.collectAsState()
         val stateAndDispatch = stateMachine.rememberStateAndDispatch()
-        var skipVerification by remember { mutableStateOf(false) }
-        val needsVerification by sessionVerificationService.needsVerificationFlow.collectAsState()
+        val skipVerification by sessionPreferencesStore.isSessionVerificationSkipped().collectAsState(initial = false)
+        val needsVerification by sessionVerificationService.canVerifySessionFlow.collectAsState(initial = true)
         val verificationFlowStep by remember {
             derivedStateOf {
                 when {
@@ -86,8 +87,7 @@ class VerifySelfSessionPresenter @Inject constructor(
                 VerifySelfSessionViewEvents.Cancel -> stateAndDispatch.dispatchAction(StateMachineEvent.Cancel)
                 VerifySelfSessionViewEvents.Reset -> stateAndDispatch.dispatchAction(StateMachineEvent.Reset)
                 VerifySelfSessionViewEvents.SkipVerification -> coroutineScope.launch {
-                    sessionVerificationService.saveVerifiedState(true)
-                    skipVerification = true
+                    sessionPreferencesStore.setSkipSessionVerification(true)
                 }
             }
         }

diff --git a/.../kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionPresenterTests.kt b/.../kotlin/io/element/android/features/verifysession/impl/VerifySelfSessionPresenterTests.kt
@@ -24,6 +24,7 @@ import com.google.common.truth.Truth.assertThat
 import io.element.android.features.verifysession.impl.VerifySelfSessionState.VerificationStep
 import io.element.android.libraries.architecture.AsyncData
 import io.element.android.libraries.core.meta.BuildMeta
+import io.element.android.libraries.featureflag.test.InMemorySessionPreferencesStore
 import io.element.android.libraries.matrix.api.encryption.EncryptionService
 import io.element.android.libraries.matrix.api.encryption.RecoveryState
 import io.element.android.libraries.matrix.api.verification.SessionVerificationData
@@ -35,7 +36,6 @@ import io.element.android.libraries.matrix.test.core.aBuildMeta
 import io.element.android.libraries.matrix.test.encryption.FakeEncryptionService
 import io.element.android.libraries.matrix.test.verification.FakeSessionVerificationService
 import io.element.android.tests.testutils.WarmUpRule
-import io.element.android.tests.testutils.lambda.value
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.test.runTest
 import org.junit.Rule
@@ -289,20 +289,23 @@ class VerifySelfSessionPresenterTests {
         }.test {
             val state = requestVerificationAndAwaitVerifyingState(service)
             state.eventSink(VerifySelfSessionViewEvents.SkipVerification)
-            service.saveVerifiedStateResult.assertions().isCalledOnce().with(value(true))
             assertThat(awaitItem().verificationFlowStep).isEqualTo(VerificationStep.Skipped)
         }
     }
 
     @Test
     fun `present - When verification is not needed, the flow is completed`() = runTest {
         val service = FakeSessionVerificationService().apply {
-            givenNeedsVerification(false)
+            givenCanVerifySession(false)
+            givenIsReady(true)
+            givenVerifiedStatus(SessionVerifiedStatus.Verified)
+            givenVerificationFlowState(VerificationFlowState.Finished)
         }
         val presenter = createVerifySelfSessionPresenter(service)
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
         }.test {
+            skipItems(1)
             assertThat(awaitItem().verificationFlowStep).isEqualTo(VerificationStep.Completed)
         }
     }
@@ -334,20 +337,21 @@ class VerifySelfSessionPresenterTests {
     private fun unverifiedSessionService(): FakeSessionVerificationService {
         return FakeSessionVerificationService().apply {
             givenVerifiedStatus(SessionVerifiedStatus.NotVerified)
-            givenNeedsVerification(true)
         }
     }
 
     private fun createVerifySelfSessionPresenter(
         service: SessionVerificationService = unverifiedSessionService(),
         encryptionService: EncryptionService = FakeEncryptionService(),
         buildMeta: BuildMeta = aBuildMeta(),
+        sessionPreferencesStore: InMemorySessionPreferencesStore = InMemorySessionPreferencesStore(),
     ): VerifySelfSessionPresenter {
         return VerifySelfSessionPresenter(
             sessionVerificationService = service,
             encryptionService = encryptionService,
             stateMachine = VerifySelfSessionStateMachine(service, encryptionService),
             buildMeta = buildMeta,
+            sessionPreferencesStore = sessionPreferencesStore,
         )
     }
 }
diff --git a/...kotlin/io/element/android/libraries/matrix/api/verification/SessionVerificationService.kt b/...kotlin/io/element/android/libraries/matrix/api/verification/SessionVerificationService.kt
@@ -21,17 +21,6 @@ import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.StateFlow
 
 interface SessionVerificationService {
-    /**
-     * This flow stores the local verification status of the current session.
-     *
-     * We should ideally base the verified status in the Rust SDK info, but there are several issues with that approach:
-     *
-     * - The SDK takes a while to report this value, resulting in a delay of 1-2s in displaying the UI.
-     * - We need to add a 'Skip' option for testing purposes, which would not be possible if we relied only on the SDK.
-     * - The SDK sometimes doesn't report the verification state if there is no network connection when the app boots.
-     */
-    val needsVerificationFlow: StateFlow<Boolean>
-
     /**
      * State of the current verification flow ([VerificationFlowState.Initial] if not started).
      */
@@ -83,11 +72,6 @@ interface SessionVerificationService {
      * Returns the verification service state to the initial step.
      */
     suspend fun reset()
-
-    /**
-     * Saves the current session state as [verified].
-     */
-    suspend fun saveVerifiedState(verified: Boolean)
 }
 
 /** Verification status of the current session. */

diff --git a/.../matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt b/.../matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/RustMatrixClient.kt
@@ -160,7 +160,6 @@ class RustMatrixClient(
         syncService = rustSyncService,
         sessionCoroutineScope = sessionCoroutineScope,
         dispatchers = dispatchers,
-        sessionStore = sessionStore,
     )
 
     private val roomDirectoryService = RustRoomDirectoryService(
@@ -188,7 +187,6 @@ class RustMatrixClient(
                             isTokenValid = false,
                             loginType = existingData.loginType,
                             passphrase = existingData.passphrase,
-                            needsVerification = existingData.needsVerification,
                         )
                         sessionStore.updateData(newData)
                         Timber.d("Removed session data with token: '...$anonymizedToken'.")
@@ -216,7 +214,6 @@ class RustMatrixClient(
                     isTokenValid = true,
                     loginType = existingData.loginType,
                     passphrase = existingData.passphrase,
-                    needsVerification = existingData.needsVerification,
                 )
                 sessionStore.updateData(newData)
                 Timber.d("Saved new session data with token: '...$anonymizedToken'.")
@@ -242,7 +239,6 @@ class RustMatrixClient(
         client = client,
         isSyncServiceReady = rustSyncService.syncState.map { it == SyncState.Running },
         sessionCoroutineScope = sessionCoroutineScope,
-        sessionStore = sessionStore,
     )
 
     private val eventFilters = TimelineConfig.excludedEvents

diff --git a/...n/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt b/...n/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt
@@ -138,7 +138,6 @@ class RustMatrixAuthenticationService @Inject constructor(
                         isTokenValid = true,
                         loginType = LoginType.PASSWORD,
                         passphrase = pendingPassphrase,
-                        needsVerification = true,
                     )
                 }
                 sessionStore.storeData(sessionData)
@@ -187,7 +186,6 @@ class RustMatrixAuthenticationService @Inject constructor(
                         isTokenValid = true,
                         loginType = LoginType.OIDC,
                         passphrase = pendingPassphrase,
-                        needsVerification = true,
                     )
                 }
                 pendingOidcAuthenticationData?.close()