mirror of
https://github.com/Novaloop-AG/element-x-android.git
synced 2026-10-31 16:22:49 +00:00
Use one RustClientSessionDelegate per client, so that an auth error only affects the session it belongs to
The RustMatrixClientFactory was using a single RustClientSessionDelegate for all the clients. The delegate keeps a reference to the last bound client only, and didReceiveAuthError does not tell which session is concerned. With several accounts, when the session of one account was revoked, the session of the last restored account was invalidated and its local data deleted instead, while the revoked session kept on syncing with an invalid token. Also the isLoggingOut flag was never reset, so any later auth error was ignored. The clients which are not associated to a stored session (temporary clients and clients used to perform a login) now use a NoOpClientSessionDelegate. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+25
@@ -0,0 +1,25 @@
|
|||||||
|
/*
|
||||||
|
* Copyright (c) 2026 Element Creations Ltd.
|
||||||
|
*
|
||||||
|
* SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Element-Commercial.
|
||||||
|
* Please see LICENSE files in the repository root for full details.
|
||||||
|
*/
|
||||||
|
|
||||||
|
package io.element.android.libraries.matrix.impl
|
||||||
|
|
||||||
|
import org.matrix.rustcomponents.sdk.ClientSessionDelegate
|
||||||
|
import org.matrix.rustcomponents.sdk.Session
|
||||||
|
|
||||||
|
/**
|
||||||
|
* A [ClientSessionDelegate] which does nothing, to be used for the clients which are not associated to a stored session,
|
||||||
|
* for instance the temporary clients, or the clients used to perform a login. The session is only stored once the login
|
||||||
|
* is done, with a client created by [RustMatrixClientFactory.create], which uses a [RustClientSessionDelegate].
|
||||||
|
*/
|
||||||
|
internal object NoOpClientSessionDelegate : ClientSessionDelegate {
|
||||||
|
override fun saveSessionInKeychain(session: Session) = Unit
|
||||||
|
|
||||||
|
override fun retrieveSessionFromKeychain(userId: String): Session {
|
||||||
|
// This should never be called, as it's only used for multi-process setups
|
||||||
|
error("retrieveSessionFromKeychain should never be called for Android")
|
||||||
|
}
|
||||||
|
}
|
||||||
+1
@@ -37,6 +37,7 @@ private val loggerTag = LoggerTag("RustClientSessionDelegate")
|
|||||||
* It implements both [ClientSessionDelegate] and [ClientDelegate] to react to session data updates and auth errors.
|
* It implements both [ClientSessionDelegate] and [ClientDelegate] to react to session data updates and auth errors.
|
||||||
*
|
*
|
||||||
* IMPORTANT: you must set the [client] property as soon as possible so [didReceiveAuthError] can work properly.
|
* IMPORTANT: you must set the [client] property as soon as possible so [didReceiveAuthError] can work properly.
|
||||||
|
* Since [didReceiveAuthError] does not tell which session is concerned, each client must have its own instance.
|
||||||
*/
|
*/
|
||||||
class RustClientSessionDelegate(
|
class RustClientSessionDelegate(
|
||||||
private val sessionStore: SessionStore,
|
private val sessionStore: SessionStore,
|
||||||
|
|||||||
+10
-1
@@ -33,6 +33,7 @@ import io.element.android.services.analytics.api.AnalyticsService
|
|||||||
import kotlinx.coroutines.CoroutineScope
|
import kotlinx.coroutines.CoroutineScope
|
||||||
import kotlinx.coroutines.withContext
|
import kotlinx.coroutines.withContext
|
||||||
import org.matrix.rustcomponents.sdk.ClientBuilder
|
import org.matrix.rustcomponents.sdk.ClientBuilder
|
||||||
|
import org.matrix.rustcomponents.sdk.ClientSessionDelegate
|
||||||
import org.matrix.rustcomponents.sdk.CrossProcessLockConfig
|
import org.matrix.rustcomponents.sdk.CrossProcessLockConfig
|
||||||
import org.matrix.rustcomponents.sdk.RequestConfig
|
import org.matrix.rustcomponents.sdk.RequestConfig
|
||||||
import org.matrix.rustcomponents.sdk.Session
|
import org.matrix.rustcomponents.sdk.Session
|
||||||
@@ -65,7 +66,11 @@ class RustMatrixClientFactory(
|
|||||||
private val clientBuilderEnterpriseHook: ClientBuilderEnterpriseHook,
|
private val clientBuilderEnterpriseHook: ClientBuilderEnterpriseHook,
|
||||||
private val innerMatrixClientFactory: RustMatrixClient.Factory,
|
private val innerMatrixClientFactory: RustMatrixClient.Factory,
|
||||||
) {
|
) {
|
||||||
private val sessionDelegate = RustClientSessionDelegate(
|
/**
|
||||||
|
* Create a new [RustClientSessionDelegate]. Each client must have its own instance, since the delegate
|
||||||
|
* keeps a reference to the client it handles the auth errors for.
|
||||||
|
*/
|
||||||
|
private fun createSessionDelegate() = RustClientSessionDelegate(
|
||||||
sessionStore = sessionStore,
|
sessionStore = sessionStore,
|
||||||
appCoroutineScope = appCoroutineScope,
|
appCoroutineScope = appCoroutineScope,
|
||||||
analyticsService = analyticsService,
|
analyticsService = analyticsService,
|
||||||
@@ -86,11 +91,14 @@ class RustMatrixClientFactory(
|
|||||||
indexDirectory.deleteRecursively()
|
indexDirectory.deleteRecursively()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Use a dedicated delegate for this client, so that an auth error only affects this session
|
||||||
|
val sessionDelegate = createSessionDelegate()
|
||||||
val client = getBaseClientBuilder(
|
val client = getBaseClientBuilder(
|
||||||
sessionPaths = sessionPaths,
|
sessionPaths = sessionPaths,
|
||||||
clientSecret = clientSecret,
|
clientSecret = clientSecret,
|
||||||
slidingSyncType = ClientBuilderSlidingSync.Restored,
|
slidingSyncType = ClientBuilderSlidingSync.Restored,
|
||||||
isMessageSearchAvailable = isMessageSearchAvailable,
|
isMessageSearchAvailable = isMessageSearchAvailable,
|
||||||
|
sessionDelegate = sessionDelegate,
|
||||||
)
|
)
|
||||||
.homeserverUrl(sessionData.homeserverUrl)
|
.homeserverUrl(sessionData.homeserverUrl)
|
||||||
.enableAutomaticBackPagination(featureFlagService.isFeatureEnabled(FeatureFlags.AutomaticBackPagination))
|
.enableAutomaticBackPagination(featureFlagService.isFeatureEnabled(FeatureFlags.AutomaticBackPagination))
|
||||||
@@ -144,6 +152,7 @@ class RustMatrixClientFactory(
|
|||||||
clientSecret: ClientSecret?,
|
clientSecret: ClientSecret?,
|
||||||
slidingSyncType: ClientBuilderSlidingSync,
|
slidingSyncType: ClientBuilderSlidingSync,
|
||||||
isMessageSearchAvailable: Boolean,
|
isMessageSearchAvailable: Boolean,
|
||||||
|
sessionDelegate: ClientSessionDelegate,
|
||||||
): ClientBuilder {
|
): ClientBuilder {
|
||||||
return clientBuilderProvider.provide()
|
return clientBuilderProvider.provide()
|
||||||
.run {
|
.run {
|
||||||
|
|||||||
+1
@@ -35,6 +35,7 @@ class RustTemporaryMatrixClientFactory(
|
|||||||
clientSecret = null,
|
clientSecret = null,
|
||||||
slidingSyncType = ClientBuilderSlidingSync.Native,
|
slidingSyncType = ClientBuilderSlidingSync.Native,
|
||||||
isMessageSearchAvailable = false,
|
isMessageSearchAvailable = false,
|
||||||
|
sessionDelegate = NoOpClientSessionDelegate,
|
||||||
)
|
)
|
||||||
.serverName(formattedServerName)
|
.serverName(formattedServerName)
|
||||||
.build()
|
.build()
|
||||||
|
|||||||
+3
@@ -34,6 +34,7 @@ import io.element.android.libraries.matrix.api.core.UserId
|
|||||||
import io.element.android.libraries.matrix.api.paths.SessionPaths
|
import io.element.android.libraries.matrix.api.paths.SessionPaths
|
||||||
import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus
|
import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus
|
||||||
import io.element.android.libraries.matrix.impl.ClientBuilderSlidingSync
|
import io.element.android.libraries.matrix.impl.ClientBuilderSlidingSync
|
||||||
|
import io.element.android.libraries.matrix.impl.NoOpClientSessionDelegate
|
||||||
import io.element.android.libraries.matrix.impl.RustMatrixClientFactory
|
import io.element.android.libraries.matrix.impl.RustMatrixClientFactory
|
||||||
import io.element.android.libraries.matrix.impl.RustTemporaryMatrixClient
|
import io.element.android.libraries.matrix.impl.RustTemporaryMatrixClient
|
||||||
import io.element.android.libraries.matrix.impl.auth.qrlogin.QrErrorMapper
|
import io.element.android.libraries.matrix.impl.auth.qrlogin.QrErrorMapper
|
||||||
@@ -407,6 +408,7 @@ class RustMatrixAuthenticationService(
|
|||||||
clientSecret = pendingKey,
|
clientSecret = pendingKey,
|
||||||
slidingSyncType = ClientBuilderSlidingSync.Discovered,
|
slidingSyncType = ClientBuilderSlidingSync.Discovered,
|
||||||
isMessageSearchAvailable = isMessageSearchAvailable(),
|
isMessageSearchAvailable = isMessageSearchAvailable(),
|
||||||
|
sessionDelegate = NoOpClientSessionDelegate,
|
||||||
)
|
)
|
||||||
.config()
|
.config()
|
||||||
.build()
|
.build()
|
||||||
@@ -440,6 +442,7 @@ class RustMatrixAuthenticationService(
|
|||||||
clientSecret = pendingKey,
|
clientSecret = pendingKey,
|
||||||
slidingSyncType = ClientBuilderSlidingSync.Discovered,
|
slidingSyncType = ClientBuilderSlidingSync.Discovered,
|
||||||
isMessageSearchAvailable = isMessageSearchAvailable(),
|
isMessageSearchAvailable = isMessageSearchAvailable(),
|
||||||
|
sessionDelegate = NoOpClientSessionDelegate,
|
||||||
)
|
)
|
||||||
.serverNameOrHomeserverUrl(baseUrlOrServerName)
|
.serverNameOrHomeserverUrl(baseUrlOrServerName)
|
||||||
.build()
|
.build()
|
||||||
|
|||||||
+43
@@ -15,6 +15,8 @@ import io.element.android.libraries.matrix.api.core.SessionId
|
|||||||
import io.element.android.libraries.matrix.api.paths.SessionPaths
|
import io.element.android.libraries.matrix.api.paths.SessionPaths
|
||||||
import io.element.android.libraries.matrix.api.scanner.ContentScanner
|
import io.element.android.libraries.matrix.api.scanner.ContentScanner
|
||||||
import io.element.android.libraries.matrix.impl.auth.FakeProxyProvider
|
import io.element.android.libraries.matrix.impl.auth.FakeProxyProvider
|
||||||
|
import io.element.android.libraries.matrix.impl.fixtures.fakes.FakeFfiClient
|
||||||
|
import io.element.android.libraries.matrix.impl.fixtures.fakes.FakeFfiClientBuilder
|
||||||
import io.element.android.libraries.matrix.impl.room.FakeTimelineEventFilterFactory
|
import io.element.android.libraries.matrix.impl.room.FakeTimelineEventFilterFactory
|
||||||
import io.element.android.libraries.matrix.impl.search.RustSearchBackfillService
|
import io.element.android.libraries.matrix.impl.search.RustSearchBackfillService
|
||||||
import io.element.android.libraries.matrix.impl.search.workmanager.SearchBackfillRequestBuilder
|
import io.element.android.libraries.matrix.impl.search.workmanager.SearchBackfillRequestBuilder
|
||||||
@@ -35,6 +37,7 @@ import kotlinx.coroutines.test.TestScope
|
|||||||
import kotlinx.coroutines.test.runTest
|
import kotlinx.coroutines.test.runTest
|
||||||
import org.junit.Test
|
import org.junit.Test
|
||||||
import org.matrix.rustcomponents.sdk.Client
|
import org.matrix.rustcomponents.sdk.Client
|
||||||
|
import org.matrix.rustcomponents.sdk.ClientDelegate
|
||||||
import java.io.File
|
import java.io.File
|
||||||
import org.matrix.rustcomponents.sdk.SyncService as ClientSyncService
|
import org.matrix.rustcomponents.sdk.SyncService as ClientSyncService
|
||||||
|
|
||||||
@@ -51,6 +54,46 @@ class RustMatrixClientFactoryTest {
|
|||||||
scheduleVacuumLambda.assertions().isCalledOnce()
|
scheduleVacuumLambda.assertions().isCalledOnce()
|
||||||
result.destroy()
|
result.destroy()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `an auth error on a client only invalidates its own session`() = runTest {
|
||||||
|
val aliceSessionId = "@alice:server.org"
|
||||||
|
val bobSessionId = "@bob:server.org"
|
||||||
|
val sessionStore = InMemorySessionStore(
|
||||||
|
initialList = listOf(
|
||||||
|
aSessionData(sessionId = aliceSessionId, isTokenValid = true),
|
||||||
|
aSessionData(sessionId = bobSessionId, isTokenValid = true),
|
||||||
|
),
|
||||||
|
updateUserProfileResult = { _, _, _, _ -> },
|
||||||
|
)
|
||||||
|
val userIds = ArrayDeque(listOf(aliceSessionId, bobSessionId))
|
||||||
|
val delegates = mutableMapOf<String, ClientDelegate?>()
|
||||||
|
val sut = createRustMatrixClientFactory(
|
||||||
|
sessionStore = sessionStore,
|
||||||
|
workManagerScheduler = FakeWorkManagerScheduler(submitLambda = {}, cancelLambda = { _, _ -> }),
|
||||||
|
clientBuilderProvider = FakeClientBuilderProvider {
|
||||||
|
val userId = userIds.removeFirst()
|
||||||
|
FakeFfiClientBuilder(
|
||||||
|
buildResult = {
|
||||||
|
FakeFfiClient(
|
||||||
|
userId = userId,
|
||||||
|
withUtdHook = {},
|
||||||
|
setDelegateResult = { delegates[userId] = it },
|
||||||
|
)
|
||||||
|
}
|
||||||
|
)
|
||||||
|
},
|
||||||
|
)
|
||||||
|
val aliceClient = sut.create(sessionStore.getSession(aliceSessionId)!!)
|
||||||
|
val bobClient = sut.create(sessionStore.getSession(bobSessionId)!!)
|
||||||
|
|
||||||
|
delegates.getValue(aliceSessionId)!!.didReceiveAuthError(isSoftLogout = false)
|
||||||
|
|
||||||
|
assertThat(sessionStore.getSession(aliceSessionId)!!.isTokenValid).isFalse()
|
||||||
|
assertThat(sessionStore.getSession(bobSessionId)!!.isTokenValid).isTrue()
|
||||||
|
aliceClient.destroy()
|
||||||
|
bobClient.destroy()
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
fun TestScope.createRustMatrixClientFactory(
|
fun TestScope.createRustMatrixClientFactory(
|
||||||
|
|||||||
+5
-1
@@ -76,6 +76,7 @@ class FakeFfiClient(
|
|||||||
private val contentScannerResult: () -> ContentScanner = { FakeFfiContentScanner() },
|
private val contentScannerResult: () -> ContentScanner = { FakeFfiContentScanner() },
|
||||||
private val runSearchBackfillResult: (SearchBackfillStrategy) -> TaskHandle = { lambdaError() },
|
private val runSearchBackfillResult: (SearchBackfillStrategy) -> TaskHandle = { lambdaError() },
|
||||||
private val closeResult: () -> Unit = {},
|
private val closeResult: () -> Unit = {},
|
||||||
|
private val setDelegateResult: (ClientDelegate?) -> Unit = {},
|
||||||
) : Client(NoHandle) {
|
) : Client(NoHandle) {
|
||||||
override fun userId(): String = userId
|
override fun userId(): String = userId
|
||||||
override fun deviceId(): String = deviceId
|
override fun deviceId(): String = deviceId
|
||||||
@@ -85,7 +86,10 @@ class FakeFfiClient(
|
|||||||
override suspend fun getNotificationSettings(): NotificationSettings = notificationSettings
|
override suspend fun getNotificationSettings(): NotificationSettings = notificationSettings
|
||||||
override fun encryption(): Encryption = encryption
|
override fun encryption(): Encryption = encryption
|
||||||
override fun session(): Session = session
|
override fun session(): Session = session
|
||||||
override fun setDelegate(delegate: ClientDelegate?): TaskHandle = FakeFfiTaskHandle()
|
override fun setDelegate(delegate: ClientDelegate?): TaskHandle {
|
||||||
|
setDelegateResult(delegate)
|
||||||
|
return FakeFfiTaskHandle()
|
||||||
|
}
|
||||||
override suspend fun cachedAvatarUrl(): String? = null
|
override suspend fun cachedAvatarUrl(): String? = null
|
||||||
override suspend fun restoreSession(session: Session) = Unit
|
override suspend fun restoreSession(session: Session) = Unit
|
||||||
override fun syncService(): SyncServiceBuilder = FakeFfiSyncServiceBuilder()
|
override fun syncService(): SyncServiceBuilder = FakeFfiSyncServiceBuilder()
|
||||||
|
|||||||
Reference in New Issue
Block a user