diff --git a/app/src/main/java/com/nextcloud/talk/data/user/UsersDao.kt b/app/src/main/java/com/nextcloud/talk/data/user/UsersDao.kt index e2291f49be6..8b83c20e613 100644 --- a/app/src/main/java/com/nextcloud/talk/data/user/UsersDao.kt +++ b/app/src/main/java/com/nextcloud/talk/data/user/UsersDao.kt @@ -22,15 +22,17 @@ import io.reactivex.Single @Dao @Suppress("TooManyFunctions") abstract class UsersDao { - // get active user - @Query("SELECT * FROM User where current = 1") + // get active user. ORDER BY/LIMIT make this deterministic if more than one row is ever + // marked current=1 (e.g. a duplicate-account row left over from a past bug), instead of + // relying on whatever order an unordered full-table scan happens to return. + @Query("SELECT * FROM User where current = 1 ORDER BY id DESC LIMIT 1") abstract fun getActiveUser(): Maybe // get active user - @Query("SELECT * FROM User where current = 1") + @Query("SELECT * FROM User where current = 1 ORDER BY id DESC LIMIT 1") abstract fun getActiveUserObservable(): Observable - @Query("SELECT * FROM User where current = 1") + @Query("SELECT * FROM User where current = 1 ORDER BY id DESC LIMIT 1") abstract fun getActiveUserSynchronously(): UserEntity? @Delete diff --git a/app/src/main/java/com/nextcloud/talk/users/UserManager.kt b/app/src/main/java/com/nextcloud/talk/users/UserManager.kt index 8aa5b3f2d66..7ec21adbbf6 100644 --- a/app/src/main/java/com/nextcloud/talk/users/UserManager.kt +++ b/app/src/main/java/com/nextcloud/talk/users/UserManager.kt @@ -76,22 +76,34 @@ class UserManager internal constructor(private val userRepository: UsersReposito /** * If there is more than one local User row for the same username+baseUrl (e.g. reusing the - * same token): Keep the current user if it's one of the duplicates, otherwise the oldest (lowest id) row, - * and schedules the rest for deletion so AccountRemovalWorker cleans them up like any other removed account. + * same token): keep whichever row [UsersRepository.getActiveUser] actually resolves to if + * it's one of the duplicates, otherwise a duplicate marked current, otherwise the oldest + * (lowest id) row, and schedules the rest for deletion so AccountRemovalWorker cleans them up + * like any other removed account. + * + * The active user is resolved first, rather than trusting each row's own `current` flag, + * because a past(?) bug could leave more than one row marked current=true for the same account. + * Picking a duplicate to keep by its `current` flag alone could then disagree with whichever + * row a live session/background sync is still bound to, and deleting that row here would trip + * a foreign key constraint on any in-flight write still referencing it. * * @return the number of duplicate rows scheduled for deletion */ fun scheduleDuplicateAccountsForDeletion(): Single = - users.map { allUsers -> - allUsers + Single.zip( + users, + userRepository.getActiveUser().map { it.id }.toSingle(NO_ACTIVE_USER_ID) + ) { allUsers, activeUserId -> + val duplicateGroups = allUsers .filter { !it.username.isNullOrEmpty() && !it.baseUrl.isNullOrEmpty() } .groupBy { it.username to it.baseUrl } .values .filter { it.size > 1 } - }.map { duplicateGroups -> + var scheduledCount = 0 duplicateGroups.forEach { duplicates -> - val userToKeep = duplicates.firstOrNull { it.current } + val userToKeep = duplicates.firstOrNull { it.id == activeUserId } + ?: duplicates.firstOrNull { it.current } ?: duplicates.minByOrNull { it.id ?: Long.MAX_VALUE } duplicates .filter { it.id != userToKeep?.id } @@ -248,6 +260,7 @@ class UserManager internal constructor(private val userRepository: UsersReposito companion object { const val TAG = "UserManager" + private const val NO_ACTIVE_USER_ID = -1L } data class UserAttributes( diff --git a/app/src/test/java/com/nextcloud/talk/users/UserManagerTest.kt b/app/src/test/java/com/nextcloud/talk/users/UserManagerTest.kt index 0c38944db5d..abc4f94b559 100644 --- a/app/src/test/java/com/nextcloud/talk/users/UserManagerTest.kt +++ b/app/src/test/java/com/nextcloud/talk/users/UserManagerTest.kt @@ -8,10 +8,12 @@ package com.nextcloud.talk.users import com.nextcloud.talk.data.user.UsersRepository import com.nextcloud.talk.data.user.model.User +import io.reactivex.Maybe import io.reactivex.Single import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue +import org.junit.Before import org.junit.Test import org.mockito.kotlin.mock import org.mockito.kotlin.verify @@ -25,6 +27,14 @@ class UserManagerTest { private fun user(id: Long, username: String, baseUrl: String, current: Boolean = false) = User(id = id, username = username, baseUrl = baseUrl, current = current) + @Before + fun setUp() { + // No row resolves as "the" active user unless a test overrides this, so + // scheduleDuplicateAccountsForDeletion() falls back to the `current` flag / oldest row, + // matching the behavior asserted by the tests below that don't care about this priority. + whenever(usersRepository.getActiveUser()).thenReturn(Maybe.empty()) + } + @Test fun `keeps the current user among duplicates and schedules the rest for deletion`() { val current = user(id = 2, username = "userA", baseUrl = "https://example.com", current = true) @@ -135,4 +145,23 @@ class UserManagerTest { assertEquals(0, scheduledCount) } + + @Test + fun `keeps whichever row getActiveUser resolves to, even over a different row flagged current`() { + // Simulates a past bug leaving two rows marked current=true for the same account: the + // active-user lookup (deterministically) resolves to one of them, but the other still + // carries the current flag too. The actively-resolved row must win, since it may be the + // one a live session/background sync is still bound to. + val staleCurrentFlag = user(id = 1, username = "userA", baseUrl = "https://example.com", current = true) + val actuallyActive = user(id = 2, username = "userA", baseUrl = "https://example.com", current = true) + whenever(usersRepository.getUsers()).thenReturn(Single.just(listOf(staleCurrentFlag, actuallyActive))) + whenever(usersRepository.getActiveUser()).thenReturn(Maybe.just(actuallyActive)) + + val scheduledCount = userManager.scheduleDuplicateAccountsForDeletion().blockingGet() + + assertEquals(1, scheduledCount) + assertTrue(staleCurrentFlag.scheduledForDeletion) + assertFalse(actuallyActive.scheduledForDeletion) + verify(usersRepository).updateUser(staleCurrentFlag) + } }