From 579ec9bd6a732d46ef12e42121f3c1bb614f05d4 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 4 Sep 2026 17:23:29 +0200 Subject: [PATCH] Close the client used to log in before creating the client of the session Since the login flows stopped handing their client over to RustMatrixClientFactory and let finalizeClientCreation() build a fresh one from the SessionData, two problems appeared. The client built by loginWithQrCode() was never owned by anyone: it is not assigned to currentClient, so clear() had nothing to close and the Rust Client stayed open for the whole app lifetime, holding the session stores of the freshly logged in account. It is now tracked in currentClient, and also disposed of when the QR login fails, which is safe because that flow always builds its own client. The client of the session was also built while the login client was still open. Both use the same session paths, so their state and crypto SQLite stores, and the search index when message search is enabled, were opened twice at the same time with no cross process lock to protect them. clear() now runs before the final client is created. Co-Authored-By: Claude Opus 5 (1M context) --- .../auth/RustMatrixAuthenticationService.kt | 10 +- .../RustMatrixAuthenticationServiceTest.kt | 106 +++++++++++++++++- .../impl/fixtures/fakes/FakeFfiClient.kt | 12 ++ .../fakes/FakeFfiLoginWithQrCodeHandler.kt | 37 ++++++ .../impl/fixtures/fakes/FakeFfiQrCodeData.kt | 5 + 5 files changed, 166 insertions(+), 4 deletions(-) create mode 100644 libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiLoginWithQrCodeHandler.kt diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt index e0758b590d..771bc60a05 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationService.kt @@ -337,6 +337,7 @@ class RustMatrixAuthenticationService( sessionPaths = emptySessionPaths, qrCodeData = sdkQrCodeLoginData, ) + currentClient = client client.newLoginWithQrCodeHandler( oauthConfiguration = oAuthConfiguration, ).use { @@ -362,6 +363,8 @@ class RustMatrixAuthenticationService( else -> it } }.onFailure { throwable -> + // A QR code login always builds its own client, so it can be disposed of on failure. + clear() if (throwable is CancellationException) { throw throwable } @@ -370,6 +373,10 @@ class RustMatrixAuthenticationService( } private suspend fun finalizeClientCreation(sessionData: SessionData): SessionId { + // Close the client which was used to perform the login before creating the final client. + // Both use the same session paths, so their SQLite stores must never be opened at the same time. + clear() + val matrixClient = restoreSession(sessionData) // Apply enterprise hooks to the newly created client as soon as possible clientEnterpriseHook(matrixClient) @@ -379,9 +386,6 @@ class RustMatrixAuthenticationService( newMatrixClientObservers.forEach { it.invoke(matrixClient) } sessionStore.addSession(sessionData) - // Clean up the strong reference held here since it's no longer necessary - clear() - return SessionId(sessionData.userId) } diff --git a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationServiceTest.kt b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationServiceTest.kt index 5d66e4f4bd..fb4fb80e1e 100644 --- a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationServiceTest.kt +++ b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/auth/RustMatrixAuthenticationServiceTest.kt @@ -14,20 +14,29 @@ import io.element.android.features.enterprise.test.FakeEnterpriseService import io.element.android.libraries.featureflag.test.FakeFeatureFlagService import io.element.android.libraries.matrix.impl.ClientBuilderProvider import io.element.android.libraries.matrix.impl.FakeClientBuilderProvider +import io.element.android.libraries.matrix.impl.auth.qrlogin.SdkQrCodeLoginData import io.element.android.libraries.matrix.impl.createRustMatrixClientFactory 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.fixtures.fakes.FakeFfiHomeserverLoginDetails +import io.element.android.libraries.matrix.impl.fixtures.fakes.FakeFfiLoginWithQrCodeHandler +import io.element.android.libraries.matrix.impl.fixtures.fakes.FakeFfiQrCodeData import io.element.android.libraries.matrix.impl.paths.SessionPathsFactory +import io.element.android.libraries.matrix.test.A_HOMESERVER_URL +import io.element.android.libraries.matrix.test.A_USER_ID import io.element.android.libraries.matrix.test.auth.FakeOAuthRedirectUrlProvider import io.element.android.libraries.matrix.test.core.aBuildMeta import io.element.android.libraries.sessionstorage.api.SessionStore import io.element.android.libraries.sessionstorage.test.InMemorySessionStore +import io.element.android.libraries.workmanager.test.FakeWorkManagerScheduler import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.testCoroutineDispatchers import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.runTest import org.junit.Test +import org.matrix.rustcomponents.sdk.Client +import org.matrix.rustcomponents.sdk.ClientBuilder +import org.matrix.rustcomponents.sdk.HumanQrLoginException import java.io.File class RustMatrixAuthenticationServiceTest { @@ -74,8 +83,102 @@ class RustMatrixAuthenticationServiceTest { closeResult.assertions().isCalledOnce() } + @Test + fun `login closes the client used to log in before building the client of the session`() = runTest { + val events = mutableListOf() + val sut = createRustMatrixAuthenticationService( + clientBuilderProvider = FakeSequentialClientBuilderProvider( + { + events.add("build login client") + FakeFfiClient( + homeserverLoginDetailsResult = { FakeFfiHomeserverLoginDetails() }, + loginResult = { _, _ -> }, + closeResult = { events.add("close login client") }, + ) + }, + { + events.add("build session client") + FakeFfiClient(withUtdHook = {}) + }, + ), + ) + + assertThat(sut.setHomeserver("matrix.org").isSuccess).isTrue() + assertThat(sut.login("alice", "password").getOrNull()).isEqualTo(A_USER_ID) + + // The two clients share the same session paths, so the login one must be closed first. + assertThat(events).containsExactly("build login client", "close login client", "build session client").inOrder() + } + + @Test + fun `loginWithQrCode closes the client used to log in before building the client of the session`() = runTest { + val events = mutableListOf() + val sut = createRustMatrixAuthenticationService( + clientBuilderProvider = FakeSequentialClientBuilderProvider( + { + events.add("build login client") + FakeFfiClient( + newLoginWithQrCodeHandlerResult = { FakeFfiLoginWithQrCodeHandler() }, + closeResult = { events.add("close login client") }, + ) + }, + { + events.add("build session client") + FakeFfiClient(withUtdHook = {}) + }, + ), + ) + + val result = sut.loginWithQrCode(aSdkQrCodeLoginData()) {} + + assertThat(result.getOrNull()).isEqualTo(A_USER_ID) + assertThat(events).containsExactly("build login client", "close login client", "build session client").inOrder() + } + + @Test + fun `loginWithQrCode closes the client it created when the login fails`() = runTest { + val closeResult = lambdaRecorder {} + val sut = createRustMatrixAuthenticationService( + clientBuilderProvider = FakeClientBuilderProvider( + provideResult = { + FakeFfiClientBuilder( + buildResult = { + FakeFfiClient( + newLoginWithQrCodeHandlerResult = { + FakeFfiLoginWithQrCodeHandler( + scanResult = { throw HumanQrLoginException.Unknown() }, + ) + }, + closeResult = closeResult, + ) + }, + ) + }, + ), + ) + + assertThat(sut.loginWithQrCode(aSdkQrCodeLoginData()) {}.isFailure).isTrue() + closeResult.assertions().isCalledOnce() + } + + private fun aSdkQrCodeLoginData() = SdkQrCodeLoginData( + FakeFfiQrCodeData( + baseUrlResult = { A_HOMESERVER_URL }, + ) + ) + + /** + * A [ClientBuilderProvider] handing out one [Client] per call, in order. + */ + private class FakeSequentialClientBuilderProvider( + private vararg val clients: () -> Client, + ) : ClientBuilderProvider { + private var index = 0 + override fun provide(): ClientBuilder = FakeFfiClientBuilder(buildResult = clients[index++]) + } + private fun TestScope.createRustMatrixAuthenticationService( - sessionStore: SessionStore = InMemorySessionStore(), + sessionStore: SessionStore = InMemorySessionStore(updateUserProfileResult = { _, _, _ -> }), clientBuilderProvider: ClientBuilderProvider = FakeClientBuilderProvider(), enterpriseService: EnterpriseService = FakeEnterpriseService(), ): RustMatrixAuthenticationService { @@ -85,6 +188,7 @@ class RustMatrixAuthenticationServiceTest { cacheDirectory = cacheDirectory, sessionStore = sessionStore, clientBuilderProvider = clientBuilderProvider, + workManagerScheduler = FakeWorkManagerScheduler(submitLambda = {}), ) return RustMatrixAuthenticationService( sessionPathsFactory = SessionPathsFactory(baseDirectory, cacheDirectory), diff --git a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiClient.kt b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiClient.kt index 86587da6c1..8c6411197a 100644 --- a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiClient.kt +++ b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiClient.kt @@ -25,10 +25,12 @@ import org.matrix.rustcomponents.sdk.Encryption import org.matrix.rustcomponents.sdk.HomeserverCapabilities import org.matrix.rustcomponents.sdk.HomeserverLoginDetails import org.matrix.rustcomponents.sdk.IgnoredUsersListener +import org.matrix.rustcomponents.sdk.LoginWithQrCodeHandler import org.matrix.rustcomponents.sdk.NoHandle import org.matrix.rustcomponents.sdk.NotificationClient import org.matrix.rustcomponents.sdk.NotificationProcessSetup import org.matrix.rustcomponents.sdk.NotificationSettings +import org.matrix.rustcomponents.sdk.OAuthConfiguration import org.matrix.rustcomponents.sdk.ProfileListener import org.matrix.rustcomponents.sdk.PusherIdentifiers import org.matrix.rustcomponents.sdk.PusherKind @@ -57,6 +59,8 @@ class FakeFfiClient( private val withUtdHook: (UnableToDecryptDelegate) -> Unit = { lambdaError() }, private val getProfileResult: (String) -> UserProfile = { aRustUserProfile() }, private val homeserverLoginDetailsResult: () -> HomeserverLoginDetails = { lambdaError() }, + private val loginResult: (String, String) -> Unit = { _, _ -> lambdaError() }, + private val newLoginWithQrCodeHandlerResult: () -> LoginWithQrCodeHandler = { lambdaError() }, private val getStoreSizesResult: () -> StoreSizes = { lambdaError() }, private val createRoomResult: (CreateRoomParameters) -> String = { lambdaError() }, private val homeserverCapabilities: HomeserverCapabilities = FakeFfiHomeserverCapabilities(), @@ -122,6 +126,14 @@ class FakeFfiClient( return homeserverLoginDetailsResult() } + override suspend fun login(username: String, password: String, initialDeviceName: String?, deviceId: String?) { + loginResult(username, password) + } + + override fun newLoginWithQrCodeHandler(oauthConfiguration: OAuthConfiguration): LoginWithQrCodeHandler { + return newLoginWithQrCodeHandlerResult() + } + override suspend fun setMediaRetentionPolicy(policy: MediaRetentionPolicy) {} override suspend fun getStoreSizes(): StoreSizes { diff --git a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiLoginWithQrCodeHandler.kt b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiLoginWithQrCodeHandler.kt new file mode 100644 index 0000000000..6e6b750643 --- /dev/null +++ b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiLoginWithQrCodeHandler.kt @@ -0,0 +1,37 @@ +/* + * 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.fixtures.fakes + +import org.matrix.rustcomponents.sdk.GeneratedQrLoginProgressListener +import org.matrix.rustcomponents.sdk.LoginWithQrCodeHandler +import org.matrix.rustcomponents.sdk.NoHandle +import org.matrix.rustcomponents.sdk.QrCodeData +import org.matrix.rustcomponents.sdk.QrLoginProgress +import org.matrix.rustcomponents.sdk.QrLoginProgressListener + +class FakeFfiLoginWithQrCodeHandler( + private val generateResult: suspend () -> Unit = {}, + private val scanResult: suspend (QrCodeData) -> Unit = {}, +) : LoginWithQrCodeHandler(NoHandle) { + private var scanProgressListener: QrLoginProgressListener? = null + + override suspend fun generate(progressListener: GeneratedQrLoginProgressListener) { + generateResult() + } + + override suspend fun scan(qrCodeData: QrCodeData, progressListener: QrLoginProgressListener) { + scanProgressListener = progressListener + scanResult(qrCodeData) + } + + fun emitScanProgress(progress: QrLoginProgress) { + scanProgressListener?.onUpdate(progress) + } + + override fun close() = Unit +} diff --git a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiQrCodeData.kt b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiQrCodeData.kt index 4070d1b2cd..926cd85d30 100644 --- a/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiQrCodeData.kt +++ b/libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/fixtures/fakes/FakeFfiQrCodeData.kt @@ -14,12 +14,17 @@ import org.matrix.rustcomponents.sdk.QrCodeData class FakeFfiQrCodeData( private val serverNameResult: () -> String? = { lambdaError() }, + private val baseUrlResult: () -> String? = { lambdaError() }, private val toBytesResult: () -> ByteArray = { lambdaError() }, ) : QrCodeData(NoHandle) { override fun serverName(): String? { return serverNameResult() } + override fun baseUrl(): String? { + return baseUrlResult() + } + override fun toBytes(): ByteArray { return toBytesResult() }