Remove SessionData.needsVerification as the source of truth for session verification status (#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 <benoit@matrix.org> * Fix review comments --------- Co-authored-by: Benoit Marty <benoit@matrix.org>
This commit is contained in:
Jorge Martin Espinosa
GitHub
parent
c83712ca91
commit
2cc124bda2
26 changed files with 99 additions and 106 deletions
|
|
@ -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. */
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -25,7 +25,6 @@ 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.sync.SyncState
|
||||
import io.element.android.libraries.matrix.impl.sync.RustSyncService
|
||||
import io.element.android.libraries.sessionstorage.api.SessionStore
|
||||
import kotlinx.coroutines.CoroutineScope
|
||||
import kotlinx.coroutines.channels.awaitClose
|
||||
import kotlinx.coroutines.currentCoroutineContext
|
||||
|
|
@ -49,11 +48,10 @@ import org.matrix.rustcomponents.sdk.EnableRecoveryProgress as RustEnableRecover
|
|||
import org.matrix.rustcomponents.sdk.SteadyStateException as RustSteadyStateException
|
||||
|
||||
internal class RustEncryptionService(
|
||||
private val client: Client,
|
||||
client: Client,
|
||||
syncService: RustSyncService,
|
||||
sessionCoroutineScope: CoroutineScope,
|
||||
private val dispatchers: CoroutineDispatchers,
|
||||
private val sessionStore: SessionStore,
|
||||
) : EncryptionService {
|
||||
private val service: Encryption = client.encryption()
|
||||
|
||||
|
|
@ -188,9 +186,6 @@ internal class RustEncryptionService(
|
|||
override suspend fun recover(recoveryKey: String): Result<Unit> = withContext(dispatchers.io) {
|
||||
runCatching {
|
||||
service.recover(recoveryKey)
|
||||
val existingSession = sessionStore.getSession(client.userId())
|
||||
?: error("Failed to save verification state. No session with id ${client.userId()}")
|
||||
sessionStore.updateData(existingSession.copy(needsVerification = false))
|
||||
}.mapFailure {
|
||||
it.mapRecoveryException()
|
||||
}
|
||||
|
|
|
|||
|
|
@ -25,7 +25,6 @@ internal fun Session.toSessionData(
|
|||
isTokenValid: Boolean,
|
||||
loginType: LoginType,
|
||||
passphrase: String?,
|
||||
needsVerification: Boolean,
|
||||
) = SessionData(
|
||||
userId = userId,
|
||||
deviceId = deviceId,
|
||||
|
|
@ -38,5 +37,4 @@ internal fun Session.toSessionData(
|
|||
isTokenValid = isTokenValid,
|
||||
loginType = loginType,
|
||||
passphrase = passphrase,
|
||||
needsVerification = needsVerification,
|
||||
)
|
||||
|
|
|
|||
|
|
@ -16,7 +16,6 @@
|
|||
|
||||
package io.element.android.libraries.matrix.impl.verification
|
||||
|
||||
import io.element.android.libraries.core.bool.orFalse
|
||||
import io.element.android.libraries.core.data.tryOrNull
|
||||
import io.element.android.libraries.matrix.api.verification.SessionVerificationData
|
||||
import io.element.android.libraries.matrix.api.verification.SessionVerificationService
|
||||
|
|
@ -24,7 +23,6 @@ import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatu
|
|||
import io.element.android.libraries.matrix.api.verification.VerificationEmoji
|
||||
import io.element.android.libraries.matrix.api.verification.VerificationFlowState
|
||||
import io.element.android.libraries.matrix.impl.util.cancelAndDestroy
|
||||
import io.element.android.libraries.sessionstorage.api.SessionStore
|
||||
import kotlinx.coroutines.CoroutineScope
|
||||
import kotlinx.coroutines.delay
|
||||
import kotlinx.coroutines.flow.Flow
|
||||
|
|
@ -33,10 +31,7 @@ import kotlinx.coroutines.flow.SharingStarted
|
|||
import kotlinx.coroutines.flow.StateFlow
|
||||
import kotlinx.coroutines.flow.asStateFlow
|
||||
import kotlinx.coroutines.flow.combine
|
||||
import kotlinx.coroutines.flow.distinctUntilChanged
|
||||
import kotlinx.coroutines.flow.first
|
||||
import kotlinx.coroutines.flow.launchIn
|
||||
import kotlinx.coroutines.flow.map
|
||||
import kotlinx.coroutines.flow.onEach
|
||||
import kotlinx.coroutines.flow.stateIn
|
||||
import kotlinx.coroutines.launch
|
||||
|
|
@ -58,7 +53,6 @@ class RustSessionVerificationService(
|
|||
private val client: Client,
|
||||
isSyncServiceReady: Flow<Boolean>,
|
||||
private val sessionCoroutineScope: CoroutineScope,
|
||||
private val sessionStore: SessionStore,
|
||||
) : SessionVerificationService, SessionVerificationControllerDelegate {
|
||||
private val encryptionService: Encryption = client.encryption()
|
||||
private lateinit var verificationController: SessionVerificationController
|
||||
|
|
@ -80,11 +74,6 @@ class RustSessionVerificationService(
|
|||
}
|
||||
})
|
||||
|
||||
override val needsVerificationFlow: StateFlow<Boolean> = sessionStore.sessionsFlow()
|
||||
.map { sessions -> sessions.firstOrNull { it.userId == client.userId() }?.needsVerification.orFalse() }
|
||||
.distinctUntilChanged()
|
||||
.stateIn(sessionCoroutineScope, SharingStarted.Eagerly, false)
|
||||
|
||||
private val _verificationFlowState = MutableStateFlow<VerificationFlowState>(VerificationFlowState.Initial)
|
||||
override val verificationFlowState = _verificationFlowState.asStateFlow()
|
||||
|
||||
|
|
@ -98,6 +87,9 @@ class RustSessionVerificationService(
|
|||
}
|
||||
|
||||
init {
|
||||
// Update initial state in case sliding sync isn't ready
|
||||
updateVerificationStatus(encryptionService.verificationState())
|
||||
|
||||
isReady.onEach { isReady ->
|
||||
if (isReady) {
|
||||
Timber.d("Starting verification service")
|
||||
|
|
@ -165,7 +157,6 @@ class RustSessionVerificationService(
|
|||
}
|
||||
}
|
||||
.onSuccess {
|
||||
saveVerifiedState(true)
|
||||
updateVerificationStatus(VerificationState.VERIFIED)
|
||||
_verificationFlowState.value = VerificationFlowState.Finished
|
||||
}
|
||||
|
|
@ -195,14 +186,6 @@ class RustSessionVerificationService(
|
|||
_verificationFlowState.value = VerificationFlowState.Initial
|
||||
}
|
||||
|
||||
override suspend fun saveVerifiedState(verified: Boolean) = tryOrFail {
|
||||
val existingSession = sessionStore.getSession(client.userId())
|
||||
?: error("Failed to save verification state. No session with id ${client.userId()}")
|
||||
sessionStore.updateData(existingSession.copy(needsVerification = !verified))
|
||||
// Wait until the new state is saved
|
||||
needsVerificationFlow.first { needsVerification -> !needsVerification }
|
||||
}
|
||||
|
||||
fun destroy() {
|
||||
Timber.d("Destroying RustSessionVerificationService")
|
||||
verificationStateListenerTaskHandle.cancelAndDestroy()
|
||||
|
|
|
|||
|
|
@ -20,22 +20,17 @@ import io.element.android.libraries.matrix.api.verification.SessionVerificationD
|
|||
import io.element.android.libraries.matrix.api.verification.SessionVerificationService
|
||||
import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus
|
||||
import io.element.android.libraries.matrix.api.verification.VerificationFlowState
|
||||
import io.element.android.tests.testutils.lambda.LambdaOneParamRecorder
|
||||
import io.element.android.tests.testutils.lambda.lambdaRecorder
|
||||
import kotlinx.coroutines.flow.Flow
|
||||
import kotlinx.coroutines.flow.MutableStateFlow
|
||||
import kotlinx.coroutines.flow.StateFlow
|
||||
|
||||
class FakeSessionVerificationService(
|
||||
var saveVerifiedStateResult: LambdaOneParamRecorder<Boolean, Unit> = lambdaRecorder<Boolean, Unit> {}
|
||||
) : SessionVerificationService {
|
||||
class FakeSessionVerificationService : SessionVerificationService {
|
||||
private val _isReady = MutableStateFlow(false)
|
||||
private val _sessionVerifiedStatus = MutableStateFlow<SessionVerifiedStatus>(SessionVerifiedStatus.Unknown)
|
||||
private var _verificationFlowState = MutableStateFlow<VerificationFlowState>(VerificationFlowState.Initial)
|
||||
private var _canVerifySessionFlow = MutableStateFlow(true)
|
||||
var shouldFail = false
|
||||
|
||||
override val needsVerificationFlow: MutableStateFlow<Boolean> = MutableStateFlow(false)
|
||||
override val verificationFlowState: StateFlow<VerificationFlowState> = _verificationFlowState
|
||||
override val sessionVerifiedStatus: StateFlow<SessionVerifiedStatus> = _sessionVerifiedStatus
|
||||
override val canVerifySessionFlow: Flow<Boolean> = _canVerifySessionFlow
|
||||
|
|
@ -94,15 +89,7 @@ class FakeSessionVerificationService(
|
|||
_isReady.value = value
|
||||
}
|
||||
|
||||
fun givenNeedsVerification(value: Boolean) {
|
||||
needsVerificationFlow.value = value
|
||||
}
|
||||
|
||||
override suspend fun reset() {
|
||||
_verificationFlowState.value = VerificationFlowState.Initial
|
||||
}
|
||||
|
||||
override suspend fun saveVerifiedState(verified: Boolean) {
|
||||
saveVerifiedStateResult(verified)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -34,5 +34,8 @@ interface SessionPreferencesStore {
|
|||
suspend fun setRenderTypingNotifications(enabled: Boolean)
|
||||
fun isRenderTypingNotificationsEnabled(): Flow<Boolean>
|
||||
|
||||
suspend fun setSkipSessionVerification(skip: Boolean)
|
||||
fun isSessionVerificationSkipped(): Flow<Boolean>
|
||||
|
||||
suspend fun clear()
|
||||
}
|
||||
|
|
|
|||
|
|
@ -49,6 +49,7 @@ class DefaultSessionPreferencesStore(
|
|||
private val renderReadReceiptsKey = booleanPreferencesKey("renderReadReceipts")
|
||||
private val sendTypingNotificationsKey = booleanPreferencesKey("sendTypingNotifications")
|
||||
private val renderTypingNotificationsKey = booleanPreferencesKey("renderTypingNotifications")
|
||||
private val skipSessionVerification = booleanPreferencesKey("skipSessionVerification")
|
||||
|
||||
private val dataStoreFile = storeFile(context, sessionId)
|
||||
private val store = PreferenceDataStoreFactory.create(
|
||||
|
|
@ -86,6 +87,9 @@ class DefaultSessionPreferencesStore(
|
|||
override suspend fun setRenderTypingNotifications(enabled: Boolean) = update(renderTypingNotificationsKey, enabled)
|
||||
override fun isRenderTypingNotificationsEnabled(): Flow<Boolean> = get(renderTypingNotificationsKey) { true }
|
||||
|
||||
override suspend fun setSkipSessionVerification(skip: Boolean) = update(skipSessionVerification, skip)
|
||||
override fun isSessionVerificationSkipped(): Flow<Boolean> = get(skipSessionVerification) { false }
|
||||
|
||||
override suspend fun clear() {
|
||||
dataStoreFile.safeDelete()
|
||||
}
|
||||
|
|
|
|||
|
|
@ -26,12 +26,14 @@ class InMemorySessionPreferencesStore(
|
|||
isRenderReadReceiptsEnabled: Boolean = true,
|
||||
isSendTypingNotificationsEnabled: Boolean = true,
|
||||
isRenderTypingNotificationsEnabled: Boolean = true,
|
||||
isSessionVerificationSkipped: Boolean = false,
|
||||
) : SessionPreferencesStore {
|
||||
private val isSharePresenceEnabled = MutableStateFlow(isSharePresenceEnabled)
|
||||
private val isSendPublicReadReceiptsEnabled = MutableStateFlow(isSendPublicReadReceiptsEnabled)
|
||||
private val isRenderReadReceiptsEnabled = MutableStateFlow(isRenderReadReceiptsEnabled)
|
||||
private val isSendTypingNotificationsEnabled = MutableStateFlow(isSendTypingNotificationsEnabled)
|
||||
private val isRenderTypingNotificationsEnabled = MutableStateFlow(isRenderTypingNotificationsEnabled)
|
||||
private val isSessionVerificationSkipped = MutableStateFlow(isSessionVerificationSkipped)
|
||||
var clearCallCount = 0
|
||||
private set
|
||||
|
||||
|
|
@ -65,6 +67,14 @@ class InMemorySessionPreferencesStore(
|
|||
|
||||
override fun isRenderTypingNotificationsEnabled(): Flow<Boolean> = isRenderTypingNotificationsEnabled
|
||||
|
||||
override suspend fun setSkipSessionVerification(skip: Boolean) {
|
||||
isSessionVerificationSkipped.tryEmit(skip)
|
||||
}
|
||||
|
||||
override fun isSessionVerificationSkipped(): Flow<Boolean> {
|
||||
return isSessionVerificationSkipped
|
||||
}
|
||||
|
||||
override suspend fun clear() {
|
||||
clearCallCount++
|
||||
isSendPublicReadReceiptsEnabled.tryEmit(true)
|
||||
|
|
|
|||
|
|
@ -44,6 +44,4 @@ data class SessionData(
|
|||
val loginType: LoginType,
|
||||
/** The optional passphrase used to encrypt data in the SDK local store. */
|
||||
val passphrase: String?,
|
||||
/** Whether the session needs verification. */
|
||||
val needsVerification: Boolean,
|
||||
)
|
||||
|
|
|
|||
|
|
@ -34,7 +34,6 @@ internal fun SessionData.toDbModel(): DbSessionData {
|
|||
isTokenValid = if (isTokenValid) 1L else 0L,
|
||||
loginType = loginType.name,
|
||||
passphrase = passphrase,
|
||||
needsVerification = if (needsVerification) 1L else 0L,
|
||||
)
|
||||
}
|
||||
|
||||
|
|
@ -51,6 +50,5 @@ internal fun DbSessionData.toApiModel(): SessionData {
|
|||
isTokenValid = isTokenValid == 1L,
|
||||
loginType = LoginType.fromName(loginType ?: LoginType.UNKNOWN.name),
|
||||
passphrase = passphrase,
|
||||
needsVerification = needsVerification == 1L,
|
||||
)
|
||||
}
|
||||
|
|
|
|||
Binary file not shown.
|
|
@ -23,9 +23,7 @@ CREATE TABLE SessionData (
|
|||
isTokenValid INTEGER NOT NULL DEFAULT 1,
|
||||
loginType TEXT,
|
||||
-- added in version 5
|
||||
passphrase TEXT,
|
||||
-- added in version 6
|
||||
needsVerification INTEGER NOT NULL DEFAULT 0
|
||||
passphrase TEXT
|
||||
);
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,20 @@
|
|||
-- Migrate DB from version 6
|
||||
-- Remove DB value for verified status, we're back to using the Rust SDK as a source of truth
|
||||
|
||||
CREATE TABLE SessionData_bak (
|
||||
userId TEXT NOT NULL PRIMARY KEY,
|
||||
deviceId TEXT NOT NULL,
|
||||
accessToken TEXT NOT NULL,
|
||||
refreshToken TEXT,
|
||||
homeserverUrl TEXT NOT NULL,
|
||||
slidingSyncProxy TEXT,
|
||||
loginTimestamp INTEGER,
|
||||
oidcData TEXT,
|
||||
isTokenValid INTEGER NOT NULL DEFAULT 1,
|
||||
loginType TEXT,
|
||||
passphrase TEXT
|
||||
);
|
||||
|
||||
INSERT INTO SessionData_bak SELECT userId, deviceId, accessToken, refreshToken, homeserverUrl, slidingSyncProxy, loginTimestamp, oidcData, isTokenValid, loginType, passphrase FROM SessionData;
|
||||
DROP TABLE SessionData;
|
||||
ALTER TABLE SessionData_bak RENAME TO SessionData;
|
||||
|
|
@ -144,7 +144,6 @@ class DatabaseSessionStoreTests {
|
|||
isTokenValid = 1,
|
||||
loginType = null,
|
||||
passphrase = "aPassphrase",
|
||||
needsVerification = 1L,
|
||||
)
|
||||
val secondSessionData = SessionData(
|
||||
userId = "userId",
|
||||
|
|
@ -158,7 +157,6 @@ class DatabaseSessionStoreTests {
|
|||
isTokenValid = 1,
|
||||
loginType = null,
|
||||
passphrase = "aPassphraseAltered",
|
||||
needsVerification = 0L,
|
||||
)
|
||||
assertThat(firstSessionData.userId).isEqualTo(secondSessionData.userId)
|
||||
assertThat(firstSessionData.loginTimestamp).isNotEqualTo(secondSessionData.loginTimestamp)
|
||||
|
|
@ -179,7 +177,6 @@ class DatabaseSessionStoreTests {
|
|||
assertThat(alteredSession.loginTimestamp).isEqualTo(firstSessionData.loginTimestamp)
|
||||
assertThat(alteredSession.oidcData).isEqualTo(secondSessionData.oidcData)
|
||||
assertThat(alteredSession.passphrase).isEqualTo(secondSessionData.passphrase)
|
||||
assertThat(alteredSession.needsVerification).isEqualTo(secondSessionData.needsVerification)
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
@ -196,7 +193,6 @@ class DatabaseSessionStoreTests {
|
|||
isTokenValid = 1,
|
||||
loginType = null,
|
||||
passphrase = "aPassphrase",
|
||||
needsVerification = 1L,
|
||||
)
|
||||
val secondSessionData = SessionData(
|
||||
userId = "userIdUnknown",
|
||||
|
|
@ -210,7 +206,6 @@ class DatabaseSessionStoreTests {
|
|||
isTokenValid = 1,
|
||||
loginType = null,
|
||||
passphrase = "aPassphraseAltered",
|
||||
needsVerification = 0L,
|
||||
)
|
||||
assertThat(firstSessionData.userId).isNotEqualTo(secondSessionData.userId)
|
||||
|
||||
|
|
@ -229,6 +224,5 @@ class DatabaseSessionStoreTests {
|
|||
assertThat(notAlteredSession.loginTimestamp).isEqualTo(firstSessionData.loginTimestamp)
|
||||
assertThat(notAlteredSession.oidcData).isEqualTo(firstSessionData.oidcData)
|
||||
assertThat(notAlteredSession.passphrase).isEqualTo(firstSessionData.passphrase)
|
||||
assertThat(notAlteredSession.needsVerification).isEqualTo(firstSessionData.needsVerification)
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -31,5 +31,4 @@ internal fun aSessionData() = SessionData(
|
|||
isTokenValid = 1,
|
||||
loginType = LoginType.UNKNOWN.name,
|
||||
passphrase = null,
|
||||
needsVerification = 0L,
|
||||
)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue