Merge pull request #6595 from nextcloud/improveScheduleDuplicateAccountsForDeletion

fix(account): resolve the active user deterministically when picking which duplicate to keep

Author: mahibi

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<UserEntity>
 
     // 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<UserEntity>
 
-    @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

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<Int> =
-        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(

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)
+    }
 }