mirror of
https://github.com/Novaloop-AG/element-x-android.git
synced 2026-10-31 16:22:49 +00:00
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) <noreply@anthropic.com>
This commit is contained in:
+7
-3
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
+105
-1
@@ -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<String>()
|
||||
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<String>()
|
||||
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<Unit> {}
|
||||
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),
|
||||
|
||||
+12
@@ -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 {
|
||||
|
||||
+37
@@ -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
|
||||
}
|
||||
+5
@@ -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()
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user