Merge pull request #5371 from element-hq/feature/bma/cleanMatrixAuthenticationService

Clean MatrixAuthenticationService and SessionStore API
This commit is contained in:
Benoit Marty
2025-09-19 11:28:37 +02:00
committed by GitHub
15 changed files with 70 additions and 67 deletions

View File

@@ -46,13 +46,13 @@ import io.element.android.libraries.architecture.waitForChildAttached
import io.element.android.libraries.core.uri.ensureProtocol
import io.element.android.libraries.deeplink.api.DeeplinkData
import io.element.android.libraries.designsystem.theme.components.CircularProgressIndicator
import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService
import io.element.android.libraries.matrix.api.core.SessionId
import io.element.android.libraries.matrix.api.core.toRoomIdOrAlias
import io.element.android.libraries.matrix.api.permalink.PermalinkData
import io.element.android.libraries.oidc.api.OidcAction
import io.element.android.libraries.oidc.api.OidcActionFlow
import io.element.android.libraries.sessionstorage.api.LoggedInState
import io.element.android.libraries.sessionstorage.api.SessionStore
import kotlinx.coroutines.flow.distinctUntilChanged
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.onEach
@@ -64,7 +64,7 @@ import timber.log.Timber
class RootFlowNode(
@Assisted val buildContext: BuildContext,
@Assisted plugins: List<Plugin>,
private val authenticationService: MatrixAuthenticationService,
private val sessionStore: SessionStore,
private val accountProviderAccessControl: AccountProviderAccessControl,
private val navStateFlowFactory: RootNavStateFlowFactory,
private val matrixSessionCache: MatrixSessionCache,
@@ -152,7 +152,7 @@ class RootFlowNode(
onSuccess: (SessionId) -> Unit,
onFailure: () -> Unit
) {
val latestSessionId = authenticationService.getLatestSessionId()
val latestSessionId = sessionStore.getLatestSessionId()
if (latestSessionId == null) {
onFailure()
return
@@ -268,7 +268,7 @@ class RootFlowNode(
private suspend fun onLoginLink(params: LoginParams) {
// Is there a session already?
val latestSessionId = authenticationService.getLatestSessionId()
val latestSessionId = sessionStore.getLatestSessionId()
if (latestSessionId == null) {
// No session, open login
if (accountProviderAccessControl.isAllowedToConnectToAccountProvider(params.accountProvider.ensureProtocol())) {
@@ -285,7 +285,7 @@ class RootFlowNode(
private suspend fun onIncomingShare(intent: Intent) {
// Is there a session already?
val latestSessionId = authenticationService.getLatestSessionId()
val latestSessionId = sessionStore.getLatestSessionId()
if (latestSessionId == null) {
// No session, open login
switchToNotLoggedInFlow(null)
@@ -342,3 +342,5 @@ class RootFlowNode(
.attachSession()
}
}
private suspend fun SessionStore.getLatestSessionId() = getLatestSession()?.userId?.let(::SessionId)

View File

@@ -12,9 +12,9 @@ import com.bumble.appyx.core.state.SavedStateMap
import dev.zacsweers.metro.Inject
import io.element.android.appnav.di.MatrixSessionCache
import io.element.android.features.preferences.api.CacheService
import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService
import io.element.android.libraries.matrix.ui.media.ImageLoaderHolder
import io.element.android.libraries.preferences.api.store.SessionPreferencesStoreFactory
import io.element.android.libraries.sessionstorage.api.SessionStore
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.combine
import kotlinx.coroutines.flow.flow
@@ -28,7 +28,7 @@ private const val SAVE_INSTANCE_KEY = "io.element.android.x.RootNavStateFlowFact
*/
@Inject
class RootNavStateFlowFactory(
private val authenticationService: MatrixAuthenticationService,
private val sessionStore: SessionStore,
private val cacheService: CacheService,
private val matrixSessionCache: MatrixSessionCache,
private val imageLoaderHolder: ImageLoaderHolder,
@@ -39,7 +39,7 @@ class RootNavStateFlowFactory(
fun create(savedStateMap: SavedStateMap?): Flow<RootNavState> {
return combine(
cacheIndexFlow(savedStateMap),
authenticationService.loggedInStateFlow(),
sessionStore.loggedInStateFlow(),
) { cacheIndex, loggedInState ->
RootNavState(
cacheIndex = cacheIndex,

View File

@@ -13,14 +13,9 @@ import io.element.android.libraries.matrix.api.auth.external.ExternalSession
import io.element.android.libraries.matrix.api.auth.qrlogin.MatrixQrCodeLoginData
import io.element.android.libraries.matrix.api.auth.qrlogin.QrCodeLoginStep
import io.element.android.libraries.matrix.api.core.SessionId
import io.element.android.libraries.sessionstorage.api.LoggedInState
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.StateFlow
interface MatrixAuthenticationService {
fun loggedInStateFlow(): Flow<LoggedInState>
suspend fun getLatestSessionId(): SessionId?
/**
* Restore a session from a [sessionId].
* Do not restore anything it the access token is not valid anymore.

View File

@@ -33,11 +33,9 @@ import io.element.android.libraries.matrix.impl.keys.PassphraseGenerator
import io.element.android.libraries.matrix.impl.mapper.toSessionData
import io.element.android.libraries.matrix.impl.paths.SessionPaths
import io.element.android.libraries.matrix.impl.paths.SessionPathsFactory
import io.element.android.libraries.sessionstorage.api.LoggedInState
import io.element.android.libraries.sessionstorage.api.LoginType
import io.element.android.libraries.sessionstorage.api.SessionStore
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.withContext
@@ -83,14 +81,6 @@ class RustMatrixAuthenticationService(
.also { sessionPaths = it }
}
override fun loggedInStateFlow(): Flow<LoggedInState> {
return sessionStore.isLoggedIn()
}
override suspend fun getLatestSessionId(): SessionId? = withContext(coroutineDispatchers.io) {
sessionStore.getLatestSession()?.userId?.let { SessionId(it) }
}
override suspend fun restoreSession(sessionId: SessionId): Result<MatrixClient> = withContext(coroutineDispatchers.io) {
runCatchingExceptions {
val sessionData = sessionStore.getSession(sessionId.value)
@@ -158,7 +148,7 @@ class RustMatrixAuthenticationService(
)
val matrixClient = rustMatrixClientFactory.create(client)
newMatrixClientObservers.forEach { it.invoke(matrixClient) }
sessionStore.storeData(sessionData)
sessionStore.addSession(sessionData)
// Clean up the strong reference held here since it's no longer necessary
currentClient = null
@@ -182,7 +172,7 @@ class RustMatrixAuthenticationService(
sessionPaths = currentSessionPaths,
)
clear()
sessionStore.storeData(sessionData)
sessionStore.addSession(sessionData)
SessionId(sessionData.userId)
}
}
@@ -250,7 +240,7 @@ class RustMatrixAuthenticationService(
val matrixClient = rustMatrixClientFactory.create(client)
newMatrixClientObservers.forEach { it.invoke(matrixClient) }
sessionStore.storeData(sessionData)
sessionStore.addSession(sessionData)
// Clean up the strong reference held here since it's no longer necessary
currentClient = null
@@ -295,7 +285,7 @@ class RustMatrixAuthenticationService(
)
val matrixClient = rustMatrixClientFactory.create(client)
newMatrixClientObservers.forEach { it.invoke(matrixClient) }
sessionStore.storeData(sessionData)
sessionStore.addSession(sessionData)
// Clean up the strong reference held here since it's no longer necessary
currentClient = null

View File

@@ -10,8 +10,10 @@ package io.element.android.libraries.matrix.impl
import io.element.android.libraries.matrix.impl.fixtures.fakes.FakeFfiClientBuilder
import org.matrix.rustcomponents.sdk.ClientBuilder
class FakeClientBuilderProvider : ClientBuilderProvider {
class FakeClientBuilderProvider(
private val provideResult: () -> ClientBuilder = { FakeFfiClientBuilder() }
) : ClientBuilderProvider {
override fun provide(): ClientBuilder {
return FakeFfiClientBuilder()
return provideResult()
}
}

View File

@@ -39,6 +39,7 @@ fun TestScope.createRustMatrixClientFactory(
baseDirectory: File = File("/base"),
cacheDirectory: File = File("/cache"),
sessionStore: SessionStore = InMemorySessionStore(),
clientBuilderProvider: ClientBuilderProvider = FakeClientBuilderProvider(),
) = RustMatrixClientFactory(
baseDirectory = baseDirectory,
cacheDirectory = cacheDirectory,
@@ -52,5 +53,5 @@ fun TestScope.createRustMatrixClientFactory(
analyticsService = FakeAnalyticsService(),
featureFlagService = FakeFeatureFlagService(),
timelineEventTypeFilterFactory = FakeTimelineEventTypeFilterFactory(),
clientBuilderProvider = FakeClientBuilderProvider(),
clientBuilderProvider = clientBuilderProvider,
)

View File

@@ -8,14 +8,17 @@
package io.element.android.libraries.matrix.impl.auth
import com.google.common.truth.Truth.assertThat
import io.element.android.libraries.matrix.api.core.SessionId
import io.element.android.libraries.matrix.impl.ClientBuilderProvider
import io.element.android.libraries.matrix.impl.FakeClientBuilderProvider
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.paths.SessionPathsFactory
import io.element.android.libraries.matrix.test.auth.FakeOidcRedirectUrlProvider
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.sessionstorage.test.aSessionData
import io.element.android.tests.testutils.testCoroutineDispatchers
import kotlinx.coroutines.test.TestScope
import kotlinx.coroutines.test.runTest
@@ -24,18 +27,28 @@ import java.io.File
class RustMatrixAuthenticationServiceTest {
@Test
fun `getLatestSessionId should return the value from the store`() = runTest {
val sessionStore = InMemorySessionStore()
fun `setHomeserver is successful`() = runTest {
val sut = createRustMatrixAuthenticationService(
sessionStore = sessionStore,
clientBuilderProvider = FakeClientBuilderProvider(
provideResult = {
FakeFfiClientBuilder(
buildResult = {
FakeFfiClient(
homeserverLoginDetailsResult = {
FakeFfiHomeserverLoginDetails()
}
)
}
)
}
),
)
assertThat(sut.getLatestSessionId()).isNull()
sessionStore.storeData(aSessionData(sessionId = "@alice:server.org"))
assertThat(sut.getLatestSessionId()).isEqualTo(SessionId("@alice:server.org"))
assertThat(sut.setHomeserver("matrix.org").isSuccess).isTrue()
}
private fun TestScope.createRustMatrixAuthenticationService(
sessionStore: SessionStore = InMemorySessionStore(),
clientBuilderProvider: ClientBuilderProvider = FakeClientBuilderProvider(),
): RustMatrixAuthenticationService {
val baseDirectory = File("/base")
val cacheDirectory = File("/cache")
@@ -43,6 +56,7 @@ class RustMatrixAuthenticationServiceTest {
baseDirectory = baseDirectory,
cacheDirectory = cacheDirectory,
sessionStore = sessionStore,
clientBuilderProvider = clientBuilderProvider,
)
return RustMatrixAuthenticationService(
sessionPathsFactory = SessionPathsFactory(baseDirectory, cacheDirectory),

View File

@@ -15,6 +15,7 @@ import io.element.android.tests.testutils.simulateLongTask
import org.matrix.rustcomponents.sdk.Client
import org.matrix.rustcomponents.sdk.ClientDelegate
import org.matrix.rustcomponents.sdk.Encryption
import org.matrix.rustcomponents.sdk.HomeserverLoginDetails
import org.matrix.rustcomponents.sdk.IgnoredUsersListener
import org.matrix.rustcomponents.sdk.NoPointer
import org.matrix.rustcomponents.sdk.NotificationClient
@@ -41,6 +42,7 @@ class FakeFfiClient(
private val session: Session = aRustSession(),
private val clearCachesResult: () -> Unit = { lambdaError() },
private val withUtdHook: (UnableToDecryptDelegate) -> Unit = { lambdaError() },
private val homeserverLoginDetailsResult: () -> HomeserverLoginDetails = { lambdaError() },
private val closeResult: () -> Unit = {},
) : Client(NoPointer) {
override fun userId(): String = userId
@@ -71,6 +73,7 @@ class FakeFfiClient(
override suspend fun ignoredUsers(): List<String> {
return emptyList()
}
override fun subscribeToIgnoredUsers(listener: IgnoredUsersListener): TaskHandle {
return FakeFfiTaskHandle()
}
@@ -78,5 +81,10 @@ class FakeFfiClient(
override suspend fun getProfile(userId: String): UserProfile {
return UserProfile(userId = userId, displayName = null, avatarUrl = null)
}
override suspend fun homeserverLoginDetails(): HomeserverLoginDetails {
return homeserverLoginDetailsResult()
}
override fun close() = closeResult()
}

View File

@@ -17,7 +17,9 @@ import uniffi.matrix_sdk.BackupDownloadStrategy
import uniffi.matrix_sdk_crypto.CollectStrategy
import uniffi.matrix_sdk_crypto.DecryptionSettings
class FakeFfiClientBuilder : ClientBuilder(NoPointer) {
class FakeFfiClientBuilder(
val buildResult: () -> Client = { FakeFfiClient(withUtdHook = {}) }
) : ClientBuilder(NoPointer) {
override fun addRootCertificates(certificates: List<ByteArray>) = this
override fun autoEnableBackups(autoEnableBackups: Boolean) = this
override fun autoEnableCrossSigning(autoEnableCrossSigning: Boolean) = this
@@ -41,7 +43,5 @@ class FakeFfiClientBuilder : ClientBuilder(NoPointer) {
override fun enableShareHistoryOnInvite(enableShareHistoryOnInvite: Boolean): ClientBuilder = this
override fun threadsEnabled(enabled: Boolean, threadSubscriptions: Boolean): ClientBuilder = this
override suspend fun build(): Client {
return FakeFfiClient(withUtdHook = {})
}
override suspend fun build() = buildResult()
}

View File

@@ -19,14 +19,11 @@ import io.element.android.libraries.matrix.api.core.SessionId
import io.element.android.libraries.matrix.test.A_SESSION_ID
import io.element.android.libraries.matrix.test.A_USER_ID
import io.element.android.libraries.matrix.test.FakeMatrixClient
import io.element.android.libraries.sessionstorage.api.LoggedInState
import io.element.android.tests.testutils.lambda.lambdaError
import io.element.android.tests.testutils.lambda.lambdaRecorder
import io.element.android.tests.testutils.simulateLongTask
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.flowOf
val A_OIDC_DATA = OidcDetails(url = "a-url")
@@ -44,14 +41,6 @@ class FakeMatrixAuthenticationService(
private var matrixClient: MatrixClient? = null
private var onAuthenticationListener: ((MatrixClient) -> Unit)? = null
var getLatestSessionIdLambda: (() -> SessionId?) = { null }
override fun loggedInStateFlow(): Flow<LoggedInState> {
return flowOf(LoggedInState.NotLoggedIn)
}
override suspend fun getLatestSessionId(): SessionId? = getLatestSessionIdLambda()
override suspend fun restoreSession(sessionId: SessionId): Result<MatrixClient> {
matrixClientResult?.let {
return it.invoke(sessionId)

View File

@@ -11,9 +11,9 @@ import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.map
interface SessionStore {
fun isLoggedIn(): Flow<LoggedInState>
fun loggedInStateFlow(): Flow<LoggedInState>
fun sessionsFlow(): Flow<List<SessionData>>
suspend fun storeData(sessionData: SessionData)
suspend fun addSession(sessionData: SessionData)
/**
* Will update the session data matching the userId, except the value of loginTimestamp.

View File

@@ -33,7 +33,7 @@ class DatabaseSessionStore(
) : SessionStore {
private val sessionDataMutex = Mutex()
override fun isLoggedIn(): Flow<LoggedInState> {
override fun loggedInStateFlow(): Flow<LoggedInState> {
return database.sessionDataQueries.selectFirst()
.asFlow()
.mapToOneOrNull(dispatchers.io)
@@ -49,7 +49,7 @@ class DatabaseSessionStore(
}
}
override suspend fun storeData(sessionData: SessionData) {
override suspend fun addSession(sessionData: SessionData) {
sessionDataMutex.withLock {
database.sessionDataQueries.insertSessionData(sessionData.toDbModel())
}

View File

@@ -44,22 +44,24 @@ class DatabaseSessionStoreTest {
}
@Test
fun `storeData persists the SessionData into the DB`() = runTest {
fun `addSession persists the SessionData into the DB`() = runTest {
assertThat(database.sessionDataQueries.selectFirst().executeAsOneOrNull()).isNull()
databaseSessionStore.storeData(aSessionData.toApiModel())
databaseSessionStore.addSession(aSessionData.toApiModel())
assertThat(database.sessionDataQueries.selectFirst().executeAsOneOrNull()).isEqualTo(aSessionData)
assertThat(database.sessionDataQueries.selectAll().executeAsList().size).isEqualTo(1)
}
@Test
fun `isLoggedIn emits true while there are sessions in the DB`() = runTest {
databaseSessionStore.isLoggedIn().test {
fun `loggedInStateFlow emits LoggedIn while there are sessions in the DB`() = runTest {
databaseSessionStore.loggedInStateFlow().test {
assertThat(awaitItem()).isEqualTo(LoggedInState.NotLoggedIn)
database.sessionDataQueries.insertSessionData(aSessionData)
databaseSessionStore.addSession(aSessionData.toApiModel())
assertThat(awaitItem()).isEqualTo(LoggedInState.LoggedIn(sessionId = aSessionData.userId, isTokenValid = true))
database.sessionDataQueries.removeSession(aSessionData.userId)
// TODO add more sessions in multi-account PR.
// Remove the first session
databaseSessionStore.removeSession(aSessionData.userId)
assertThat(awaitItem()).isEqualTo(LoggedInState.NotLoggedIn)
}
}

View File

@@ -51,7 +51,7 @@ import org.junit.Test
runCurrent()
val listener = TestSessionListener()
sut.addListener(listener)
databaseSessionStore.storeData(sessionData.toApiModel())
databaseSessionStore.addSession(sessionData.toApiModel())
listener.assertEvents(TestSessionListener.Event.Created(sessionData.userId))
sut.removeListener(listener)
coroutineContext.cancelChildren()
@@ -64,7 +64,7 @@ import org.junit.Test
runCurrent()
val listener = TestSessionListener()
sut.addListener(listener)
databaseSessionStore.storeData(sessionData.toApiModel())
databaseSessionStore.addSession(sessionData.toApiModel())
listener.assertEvents(TestSessionListener.Event.Created(sessionData.userId))
databaseSessionStore.removeSession(sessionData.userId)
listener.assertEvents(

View File

@@ -20,7 +20,7 @@ class InMemorySessionStore(
) : SessionStore {
private val sessionDataListFlow = MutableStateFlow(initialList)
override fun isLoggedIn(): Flow<LoggedInState> {
override fun loggedInStateFlow(): Flow<LoggedInState> {
return sessionDataListFlow.map {
if (it.isEmpty()) {
LoggedInState.NotLoggedIn
@@ -37,7 +37,7 @@ class InMemorySessionStore(
override fun sessionsFlow(): Flow<List<SessionData>> = sessionDataListFlow.asStateFlow()
override suspend fun storeData(sessionData: SessionData) {
override suspend fun addSession(sessionData: SessionData) {
val currentList = sessionDataListFlow.value.toMutableList()
currentList.removeAll { it.userId == sessionData.userId }
currentList.add(sessionData)