From 58cbd080a1a604ada6008298da6a0aa213dee498 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Mon, 24 Aug 2026 17:16:20 +0200 Subject: [PATCH 1/2] fix(account): prevent duplicate accounts from onResume re-verification AccountVerificationActivity kicked off its login-verification network chain (capabilities -> profile -> storeProfile) from onResume(), which fires again any time the activity is resumed (e.g. a screen lock/unlock) while verification is still in flight. Each re-entry ran the whole chain again and inserted another User row with the same username/baseUrl/token, since storeProfile() always creates a new row and never checks for an existing match. Move the verification trigger into onCreate(), which runs exactly once per activity instance, so a resume mid-verification no longer restarts the flow. To reproduce: 1. Remove any existing account for the test user so the local DB has no matching User row. 2. Start "Add account" and log in normally; this lands on AccountVerificationActivity and shows "Verifying account..." while it makes several sequential network calls. 3. While that screen is still showing, lock and unlock the screen a couple of times in quick succession (e.g. `adb shell input keyevent KEYCODE_POWER` twice, repeated) to force onResume() to fire again mid-verification. 4. Once login completes, inspect the local User table (e.g. via `adb exec-out run-as cat databases/nextcloud_talk.sqlite` and sqlcipher) - before this fix, one extra row appears per resume that landed during verification, all sharing the same username/baseUrl/ token. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Marcel Hibbe Signed-off-by: Marcel Hibbe --- .../account/AccountVerificationActivity.kt | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt b/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt index bd745baccc..139d505139 100644 --- a/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/account/AccountVerificationActivity.kt @@ -98,6 +98,16 @@ class AccountVerificationActivity : BaseActivity() { initSystemBars() handleIntent() + + if ( + isAccountImport && + !UriUtils.hasHttpProtocolPrefixed(baseUrl!!) || + isNotSameProtocol(baseUrl!!, originalProtocol) + ) { + determineBaseUrlProtocol(true) + } else { + findServerTalkApp() + } } private fun handleIntent() { @@ -113,20 +123,6 @@ class AccountVerificationActivity : BaseActivity() { } } - override fun onResume() { - super.onResume() - - if ( - isAccountImport && - !UriUtils.hasHttpProtocolPrefixed(baseUrl!!) || - isNotSameProtocol(baseUrl!!, originalProtocol) - ) { - determineBaseUrlProtocol(true) - } else { - findServerTalkApp() - } - } - private fun isNotSameProtocol(baseUrl: String, originalProtocol: String?): Boolean { if (originalProtocol == null) { return true From ba4eba5caf670288ab1a7bdcd0a0d0a5c8cb44d2 Mon Sep 17 00:00:00 2001 From: Marcel Hibbe Date: Mon, 24 Aug 2026 19:35:23 +0200 Subject: [PATCH 2/2] fix(account): remove duplicate accounts created by the verification bug Add UserManager.scheduleDuplicateAccountsForDeletion(): for each set of local User rows sharing the same username+baseUrl, keep the current account if present, otherwise the oldest row, and schedule the rest for deletion. AccountRemovalWorker now runs this check before its existing removal pass, so duplicates are fully cleaned up (push unregistration, shortcuts, arbitrary storage, WebSocket instance, DB row) through the same path as any other account removal, and before WebsocketConnectionsWorker would otherwise open a parallel signaling connection per duplicate. Logs a warning via the file-backed Logger when duplicates are found, so it's visible without the user needing to have enabled logging beforehand. Add UserManagerTest covering: keeping the current vs. oldest row, groups of three or more duplicates, multiple independent duplicate groups in one pass, rows with a null/blank username or baseUrl never being grouped, and the no-duplicates/no-users no-op cases. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Marcel Hibbe --- .../talk/jobs/AccountRemovalWorker.java | 9 ++ .../com/nextcloud/talk/users/UserManager.kt | 30 ++++ .../nextcloud/talk/users/UserManagerTest.kt | 138 ++++++++++++++++++ 3 files changed, 177 insertions(+) create mode 100644 app/src/test/java/com/nextcloud/talk/users/UserManagerTest.kt diff --git a/app/src/main/java/com/nextcloud/talk/jobs/AccountRemovalWorker.java b/app/src/main/java/com/nextcloud/talk/jobs/AccountRemovalWorker.java index bbb4f4440b..6023f20e34 100644 --- a/app/src/main/java/com/nextcloud/talk/jobs/AccountRemovalWorker.java +++ b/app/src/main/java/com/nextcloud/talk/jobs/AccountRemovalWorker.java @@ -19,6 +19,7 @@ import com.nextcloud.talk.data.database.dao.ChatMessagesDao; import com.nextcloud.talk.data.database.dao.ConversationsDao; import com.nextcloud.talk.data.user.model.User; +import com.nextcloud.talk.logger.Logger; import com.nextcloud.talk.models.json.generic.GenericMeta; import com.nextcloud.talk.models.json.generic.GenericOverall; import com.nextcloud.talk.models.json.push.PushConfigurationState; @@ -66,6 +67,8 @@ public class AccountRemovalWorker extends Worker { @Inject ChatBlocksDao chatBlocksDao; + @Inject Logger logger; + NcApi ncApi; public AccountRemovalWorker(@NonNull Context context, @NonNull WorkerParameters workerParams) { @@ -77,6 +80,12 @@ public AccountRemovalWorker(@NonNull Context context, @NonNull WorkerParameters public Result doWork() { Objects.requireNonNull(NextcloudTalkApplication.Companion.getSharedApplication()).getComponentApplication().inject(this); + int duplicateAccountsScheduled = userManager.scheduleDuplicateAccountsForDeletion().blockingGet(); + if (duplicateAccountsScheduled > 0) { + logger.w(TAG, "Found and scheduled " + duplicateAccountsScheduled + + " duplicate account(s) for deletion"); + } + List users = userManager.getUsersScheduledForDeletion().blockingGet(); for (User user : users) { if (user.getPushConfigurationState() != null) { 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 326945a5b5..8aa5b3f2d6 100644 --- a/app/src/main/java/com/nextcloud/talk/users/UserManager.kt +++ b/app/src/main/java/com/nextcloud/talk/users/UserManager.kt @@ -74,6 +74,36 @@ class UserManager internal constructor(private val userRepository: UsersReposito .map { true } .switchIfEmpty(Single.just(false)) + /** + * 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. + * + * @return the number of duplicate rows scheduled for deletion + */ + fun scheduleDuplicateAccountsForDeletion(): Single = + users.map { allUsers -> + 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 } + ?: duplicates.minByOrNull { it.id ?: Long.MAX_VALUE } + duplicates + .filter { it.id != userToKeep?.id } + .forEach { duplicate -> + duplicate.scheduledForDeletion = true + userRepository.updateUser(duplicate) + scheduledCount++ + } + } + scheduledCount + } + private fun getAnyUserAndSetAsActive(): Maybe { val results = userRepository.getUsersNotScheduledForDeletion() diff --git a/app/src/test/java/com/nextcloud/talk/users/UserManagerTest.kt b/app/src/test/java/com/nextcloud/talk/users/UserManagerTest.kt new file mode 100644 index 0000000000..0c38944db5 --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/users/UserManagerTest.kt @@ -0,0 +1,138 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.users + +import com.nextcloud.talk.data.user.UsersRepository +import com.nextcloud.talk.data.user.model.User +import io.reactivex.Single +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.mockito.kotlin.mock +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever + +class UserManagerTest { + + private val usersRepository: UsersRepository = mock() + private val userManager = UserManager(usersRepository) + + private fun user(id: Long, username: String, baseUrl: String, current: Boolean = false) = + User(id = id, username = username, baseUrl = baseUrl, current = current) + + @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) + val duplicate = user(id = 1, username = "userA", baseUrl = "https://example.com", current = false) + whenever(usersRepository.getUsers()).thenReturn(Single.just(listOf(current, duplicate))) + + val scheduledCount = userManager.scheduleDuplicateAccountsForDeletion().blockingGet() + + assertEquals(1, scheduledCount) + assertTrue(duplicate.scheduledForDeletion) + assertFalse(current.scheduledForDeletion) + verify(usersRepository).updateUser(duplicate) + } + + @Test + fun `keeps the oldest row when none of the duplicates is current`() { + val oldest = user(id = 1, username = "userA", baseUrl = "https://example.com") + val newer = user(id = 2, username = "userA", baseUrl = "https://example.com") + whenever(usersRepository.getUsers()).thenReturn(Single.just(listOf(newer, oldest))) + + val scheduledCount = userManager.scheduleDuplicateAccountsForDeletion().blockingGet() + + assertEquals(1, scheduledCount) + assertTrue(newer.scheduledForDeletion) + assertFalse(oldest.scheduledForDeletion) + } + + @Test + fun `does nothing when there are no duplicates`() { + val userA = user(id = 1, username = "userA", baseUrl = "https://example.com", current = true) + val userB = user(id = 2, username = "userB", baseUrl = "https://example.com") + whenever(usersRepository.getUsers()).thenReturn(Single.just(listOf(userA, userB))) + + val scheduledCount = userManager.scheduleDuplicateAccountsForDeletion().blockingGet() + + assertEquals(0, scheduledCount) + assertFalse(userA.scheduledForDeletion) + assertFalse(userB.scheduledForDeletion) + } + + @Test + fun `different servers with the same username are not treated as duplicates`() { + val userA = user(id = 1, username = "userA", baseUrl = "https://example.com") + val userB = user(id = 2, username = "userA", baseUrl = "https://other.example.com") + whenever(usersRepository.getUsers()).thenReturn(Single.just(listOf(userA, userB))) + + val scheduledCount = userManager.scheduleDuplicateAccountsForDeletion().blockingGet() + + assertEquals(0, scheduledCount) + } + + @Test + fun `rows with a null or blank username or baseUrl are never grouped as duplicates`() { + val nullUsername = user(id = 1, username = "userA", baseUrl = "https://example.com") + .apply { username = null } + val anotherNullUsername = user(id = 2, username = "userA", baseUrl = "https://example.com") + .apply { username = null } + val blankBaseUrl = user(id = 3, username = "userA", baseUrl = "") + val anotherBlankBaseUrl = user(id = 4, username = "userA", baseUrl = "") + whenever(usersRepository.getUsers()).thenReturn( + Single.just(listOf(nullUsername, anotherNullUsername, blankBaseUrl, anotherBlankBaseUrl)) + ) + + val scheduledCount = userManager.scheduleDuplicateAccountsForDeletion().blockingGet() + + assertEquals(0, scheduledCount) + } + + @Test + fun `keeps only one row out of three or more duplicates`() { + val current = user(id = 3, username = "userA", baseUrl = "https://example.com", current = true) + val duplicate1 = user(id = 1, username = "userA", baseUrl = "https://example.com") + val duplicate2 = user(id = 2, username = "userA", baseUrl = "https://example.com") + whenever(usersRepository.getUsers()).thenReturn(Single.just(listOf(duplicate1, duplicate2, current))) + + val scheduledCount = userManager.scheduleDuplicateAccountsForDeletion().blockingGet() + + assertEquals(2, scheduledCount) + assertTrue(duplicate1.scheduledForDeletion) + assertTrue(duplicate2.scheduledForDeletion) + assertFalse(current.scheduledForDeletion) + } + + @Test + fun `handles multiple independent duplicate groups in one pass`() { + val userACurrent = user(id = 1, username = "userA", baseUrl = "https://example.com", current = true) + val userADuplicate = user(id = 2, username = "userA", baseUrl = "https://example.com") + val userBOldest = user(id = 3, username = "userB", baseUrl = "https://example.com") + val userBNewer = user(id = 4, username = "userB", baseUrl = "https://example.com") + whenever(usersRepository.getUsers()).thenReturn( + Single.just(listOf(userACurrent, userADuplicate, userBNewer, userBOldest)) + ) + + val scheduledCount = userManager.scheduleDuplicateAccountsForDeletion().blockingGet() + + assertEquals(2, scheduledCount) + assertTrue(userADuplicate.scheduledForDeletion) + assertTrue(userBNewer.scheduledForDeletion) + assertFalse(userACurrent.scheduledForDeletion) + assertFalse(userBOldest.scheduledForDeletion) + } + + @Test + fun `does nothing when there are no users at all`() { + whenever(usersRepository.getUsers()).thenReturn(Single.just(emptyList())) + + val scheduledCount = userManager.scheduleDuplicateAccountsForDeletion().blockingGet() + + assertEquals(0, scheduledCount) + } +}