diff --git a/app/src/main/java/com/nextcloud/talk/account/BrowserLoginActivity.kt b/app/src/main/java/com/nextcloud/talk/account/BrowserLoginActivity.kt index 649fb7c6e6b..fabf8a4a673 100644 --- a/app/src/main/java/com/nextcloud/talk/account/BrowserLoginActivity.kt +++ b/app/src/main/java/com/nextcloud/talk/account/BrowserLoginActivity.kt @@ -118,6 +118,13 @@ class BrowserLoginActivity : BaseActivity() { Snackbar.make(binding.root, R.string.nc_login_too_many_attempts, Snackbar.LENGTH_LONG) .show() } + BrowserLoginActivityViewModel.PostLoginViewState.PostLoginRemovalPending -> { + logger.e(TAG, "Account removal did not finish in time") + // This login can't be retried here. Back on the previous screen, it can be started again, + // so the message must outlive this activity. + Toast.makeText(context, R.string.nc_account_removal_pending, Toast.LENGTH_LONG).show() + finish() + } BrowserLoginActivityViewModel.PostLoginViewState.PostLoginRestartApp -> { restartApp() } diff --git a/app/src/main/java/com/nextcloud/talk/account/data/LoginRepository.kt b/app/src/main/java/com/nextcloud/talk/account/data/LoginRepository.kt index a51604352aa..d0f52f72798 100644 --- a/app/src/main/java/com/nextcloud/talk/account/data/LoginRepository.kt +++ b/app/src/main/java/com/nextcloud/talk/account/data/LoginRepository.kt @@ -49,9 +49,12 @@ class LoginRepository(val network: NetworkLoginDataSource, val local: LocalLogin /** A new account, to be verified and stored with the data in [bundle]. */ data class NewAccount(val bundle: Bundle) : LoginResult - /** An existing account, reauthorized or not, or an account that is being removed. */ + /** An existing account, reauthorized or not. */ data object ExistingAccount : LoginResult + /** The account is still being removed, so it can't be set up again yet. */ + data object RemovalPending : LoginResult + /** A reauthorization in which a different account than the one to reauthorize logged in. */ data object DifferentAccount : LoginResult } @@ -211,16 +214,22 @@ class LoginRepository(val network: NetworkLoginDataSource, val local: LocalLogin } /** - * Returns [LoginResult.NewAccount] if the account is not scheduled for deletion and doesn't exist yet. During a + * Returns [LoginResult.NewAccount] if the account doesn't exist yet. An account that is scheduled for deletion is + * waited for first, [LoginResult.RemovalPending] if its removal does not finish in time. During a * reauthorization, [LoginResult.DifferentAccount] if another account than the one to reauthorize logged in, set * up or not, which is then left unchanged. */ suspend fun parseAndLogin(loginData: LoginCompletion): LoginResult { if (local.checkIfUserIsScheduledForDeletion(loginData)) { - // however the user is not yet deleted, just start AccountRemovalWorker again to make sure to delete it. + // The account was removed just before and the removal did not finish yet. Make sure it runs and wait for + // it, as the account can then be set up again. Otherwise the login would end without any account. local.startAccountRemovalWorker() - return LoginResult.ExistingAccount - } else if (local.checkIfUserExists(loginData)) { + if (!local.awaitUserRemoval(loginData)) { + return LoginResult.RemovalPending + } + } + + if (local.checkIfUserExists(loginData)) { if (shouldReauthorizeUser) { if (!local.updateUser(loginData, accountToReauthorize)) { Log.w(TAG, "Logged in with a different account than the one to reauthorize. Skipped update.") diff --git a/app/src/main/java/com/nextcloud/talk/account/data/io/LocalLoginDataSource.kt b/app/src/main/java/com/nextcloud/talk/account/data/io/LocalLoginDataSource.kt index 395537520f2..b6b40ca4549 100644 --- a/app/src/main/java/com/nextcloud/talk/account/data/io/LocalLoginDataSource.kt +++ b/app/src/main/java/com/nextcloud/talk/account/data/io/LocalLoginDataSource.kt @@ -17,11 +17,18 @@ import com.nextcloud.talk.account.data.model.LoginCompletion import com.nextcloud.talk.jobs.AccountRemovalWorker import com.nextcloud.talk.users.UserManager import com.nextcloud.talk.utils.preferences.AppPreferences +import kotlinx.coroutines.delay +import kotlinx.coroutines.withTimeoutOrNull // local datasource for communicating with room through account manager // crucial for making sure the login process interacts with the db as expected. class LocalLoginDataSource(val userManager: UserManager, val appPreferences: AppPreferences, val context: Context) { + companion object { + private const val REMOVAL_TIMEOUT_MILLIS = 30_000L + private const val REMOVAL_POLL_INTERVAL_MILLIS = 200L + } + /** * Stores the new credentials of a reauthorized account. That is the account that logged in, found by its login * name and server, as the browser login flow grants an app password for whoever is logged in there. Returns false @@ -48,6 +55,19 @@ class LocalLoginDataSource(val userManager: UserManager, val appPreferences: App return WorkManager.getInstance(context).getWorkInfoByIdLiveData(accountRemovalWork.id) } + /** + * Waits until the account of [data] is not scheduled for deletion anymore, i.e. the account removal finished, for + * at most [timeoutMillis]. Returns false if it still is. The removal also contacts the server and the push proxy, + * so it can take a while. + */ + suspend fun awaitUserRemoval(data: LoginCompletion, timeoutMillis: Long = REMOVAL_TIMEOUT_MILLIS): Boolean = + withTimeoutOrNull(timeoutMillis) { + while (checkIfUserIsScheduledForDeletion(data)) { + delay(REMOVAL_POLL_INTERVAL_MILLIS) + } + true + } ?: false + suspend fun checkIfUserIsScheduledForDeletion(data: LoginCompletion): Boolean = userManager.checkIfUserIsScheduledForDeletion(data.loginName, data.server) diff --git a/app/src/main/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModel.kt b/app/src/main/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModel.kt index 9ab868f8c02..43cf3877727 100644 --- a/app/src/main/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModel.kt +++ b/app/src/main/java/com/nextcloud/talk/account/viewmodels/BrowserLoginActivityViewModel.kt @@ -41,6 +41,7 @@ class BrowserLoginActivityViewModel @Inject constructor(val repository: LoginRep data object PostLoginTooManyLoginAttempts : PostLoginViewState() data class PostLoginContinue(val data: Bundle) : PostLoginViewState() data object PostLoginDifferentAccount : PostLoginViewState() + data object PostLoginRemovalPending : PostLoginViewState() } private val _postLoginState = MutableStateFlow(PostLoginViewState.None) @@ -121,6 +122,7 @@ class BrowserLoginActivityViewModel @Inject constructor(val repository: LoginRep is LoginRepository.LoginResult.NewAccount -> PostLoginViewState.PostLoginContinue(result.bundle) LoginRepository.LoginResult.ExistingAccount -> PostLoginViewState.PostLoginRestartApp LoginRepository.LoginResult.DifferentAccount -> PostLoginViewState.PostLoginDifferentAccount + LoginRepository.LoginResult.RemovalPending -> PostLoginViewState.PostLoginRemovalPending } fun cancelLogin() = repository.cancelLoginFlow() 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 4062d2d27b4..24ea7b4444b 100644 --- a/app/src/main/java/com/nextcloud/talk/jobs/AccountRemovalWorker.java +++ b/app/src/main/java/com/nextcloud/talk/jobs/AccountRemovalWorker.java @@ -42,6 +42,7 @@ import androidx.work.Worker; import androidx.work.WorkerParameters; import autodagger.AutoInjector; +import io.reactivex.Observable; import io.reactivex.Observer; import io.reactivex.disposables.Disposable; import kotlin.coroutines.EmptyCoroutineContext; @@ -129,6 +130,9 @@ public void onNext(@io.reactivex.annotations.NonNull GenericOverall genericOvera queryMap.put("deviceIdentifierSignature", finalPushConfigurationState.getDeviceIdentifierSignature()); unregisterDeviceForNotificationWithProxy(queryMap, user); + } else { + Log.e(TAG, "unregister Device For Notifications failed with status " + statusCode); + initiateUserDeletion(user); } } @@ -152,45 +156,40 @@ public void onComplete() { } private void unregisterDeviceForNotificationWithProxy(HashMap queryMap, User user) { - ncApi.unregisterDeviceForNotificationsWithProxy - (ApiUtils.getUrlPushProxy(), queryMap) - .subscribe(new Observer() { - @Override - public void onSubscribe(Disposable d) { - // unused atm - } - - @Override - public void onNext(Void aVoid) { - String groupName = String.format( - getApplicationContext() - .getResources() - .getString(R.string.nc_notification_channel), user.getUserId(), user.getBaseUrl()); - CRC32 crc32 = new CRC32(); - crc32.update(groupName.getBytes()); - NotificationManager notificationManager = - (NotificationManager) getApplicationContext() - .getSystemService(Context.NOTIFICATION_SERVICE); - - if (notificationManager != null) { - notificationManager.deleteNotificationChannelGroup( - Long.toString(crc32.getValue())); - } - - initiateUserDeletion(user); - } + Throwable error = awaitCompletion( + ncApi.unregisterDeviceForNotificationsWithProxy(ApiUtils.getUrlPushProxy(), queryMap)); + + if (error == null) { + String groupName = String.format( + getApplicationContext() + .getResources() + .getString(R.string.nc_notification_channel), user.getUserId(), user.getBaseUrl()); + CRC32 crc32 = new CRC32(); + crc32.update(groupName.getBytes()); + NotificationManager notificationManager = + (NotificationManager) getApplicationContext() + .getSystemService(Context.NOTIFICATION_SERVICE); + + if (notificationManager != null) { + notificationManager.deleteNotificationChannelGroup( + Long.toString(crc32.getValue())); + } + } else { + Log.e(TAG, "error while trying to unregister Device For Notification With Proxy", error); + } - @Override - public void onError(Throwable e) { - Log.e(TAG, "error while trying to unregister Device For Notification With Proxy", e); - initiateUserDeletion(user); - } + initiateUserDeletion(user); + } - @Override - public void onComplete() { - // unused atm - } - }); + /** + * Blocks until the request ended, so that doWork() returns only after the user is deleted. Callers restart the + * app or log in again as soon as the work is SUCCEEDED, and the user must be gone by then. + * The elements are ignored: the body of the proxy answer is null, which a blocking subscriber can't take. + * + * @return the error, or null if the request succeeded + */ + static Throwable awaitCompletion(Observable request) { + return request.ignoreElements().blockingGet(); } private void initiateUserDeletion(User user) { diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 3be4fb559e4..c5ac7e08e72 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -33,6 +33,7 @@ How to translate with transifex: Dismiss Sorry, something went wrong! Too many failed login attempts from this network. Try again later. + The removal of this account is not finished yet. Please try again in a moment. You logged in with a different account. Log in with the account you want to reauthorize. Create Unknown diff --git a/app/src/test/java/com/nextcloud/talk/account/data/io/LocalLoginDataSourceTest.kt b/app/src/test/java/com/nextcloud/talk/account/data/io/LocalLoginDataSourceTest.kt new file mode 100644 index 00000000000..9d229fa213a --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/account/data/io/LocalLoginDataSourceTest.kt @@ -0,0 +1,46 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.account.data.io + +import com.nextcloud.talk.account.data.model.LoginCompletion +import com.nextcloud.talk.users.UserManager +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.runTest +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test +import org.mockito.kotlin.mock +import org.mockito.kotlin.times +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever + +@OptIn(ExperimentalCoroutinesApi::class) +class LocalLoginDataSourceTest { + + private val userManager: UserManager = mock() + private val dataSource = LocalLoginDataSource(userManager, mock(), mock()) + private val loginData = LoginCompletion(200, "https://example.com", "user", "pass") + + @Test + fun `awaitUserRemoval returns once the user is not scheduled for deletion anymore`() = + runTest { + whenever(userManager.checkIfUserIsScheduledForDeletion("user", "https://example.com")) + .thenReturn(true, true, false) + + assertTrue(dataSource.awaitUserRemoval(loginData, 10_000L)) + verify(userManager, times(3)).checkIfUserIsScheduledForDeletion("user", "https://example.com") + } + + @Test + fun `awaitUserRemoval gives up when the removal does not finish in time`() = + runTest { + whenever(userManager.checkIfUserIsScheduledForDeletion("user", "https://example.com")) + .thenReturn(true) + + assertFalse(dataSource.awaitUserRemoval(loginData, 1_000L)) + } +} diff --git a/app/src/test/java/com/nextcloud/talk/jobs/AccountRemovalWorkerTest.kt b/app/src/test/java/com/nextcloud/talk/jobs/AccountRemovalWorkerTest.kt new file mode 100644 index 00000000000..d57791f6a1a --- /dev/null +++ b/app/src/test/java/com/nextcloud/talk/jobs/AccountRemovalWorkerTest.kt @@ -0,0 +1,65 @@ +/* + * Nextcloud Talk - Android Client + * + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: GPL-3.0-or-later + */ +package com.nextcloud.talk.jobs + +import android.app.Application +import com.nextcloud.talk.api.NcApi +import mockwebserver3.MockResponse +import mockwebserver3.MockWebServer +import org.junit.After +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config +import retrofit2.HttpException +import retrofit2.Retrofit +import retrofit2.adapter.rxjava2.RxJava2CallAdapterFactory + +@RunWith(RobolectricTestRunner::class) +@Config(application = Application::class, sdk = [33]) +class AccountRemovalWorkerTest { + + private val server = MockWebServer() + private lateinit var ncApi: NcApi + + @Before + fun setUp() { + server.start() + ncApi = Retrofit.Builder() + .baseUrl(server.url("/")) + .addCallAdapterFactory(RxJava2CallAdapterFactory.create()) + .build() + .create(NcApi::class.java) + } + + @After + fun tearDown() { + server.close() + } + + @Test + fun `a successful request with an empty body is recognized as success`() { + // The push proxy answers without a body, Retrofit emits null. + server.enqueue(MockResponse()) + + val request = ncApi.unregisterDeviceForNotificationsWithProxy(server.url("/devices").toString(), emptyMap()) + + assertNull(AccountRemovalWorker.awaitCompletion(request)) + } + + @Test + fun `a failed request returns its error`() { + server.enqueue(MockResponse(code = 500)) + + val request = ncApi.unregisterDeviceForNotificationsWithProxy(server.url("/devices").toString(), emptyMap()) + + assertTrue(AccountRemovalWorker.awaitCompletion(request) is HttpException) + } +} diff --git a/app/src/test/java/com/nextcloud/talk/login/data/LoginRepositoryTest.kt b/app/src/test/java/com/nextcloud/talk/login/data/LoginRepositoryTest.kt index 1e74b307402..1fc9275df59 100644 --- a/app/src/test/java/com/nextcloud/talk/login/data/LoginRepositoryTest.kt +++ b/app/src/test/java/com/nextcloud/talk/login/data/LoginRepositoryTest.kt @@ -389,7 +389,7 @@ class LoginRepositoryTest { // ========== parseAndLogin() Tests ========== @Test - fun `parseAndLogin returns null when user is scheduled for deletion`() = + fun `parseAndLogin sets the account up again once its pending removal finished`() = runTest { // Arrange val loginData = LoginCompletion(200, "https://server.com", "testuser", "apppass123") @@ -397,13 +397,31 @@ class LoginRepositoryTest { .thenReturn(true) whenever(localLoginDataSource.startAccountRemovalWorker()) .thenReturn(liveData) + whenever(localLoginDataSource.awaitUserRemoval(any(), any())).thenReturn(true) + + whenever(localLoginDataSource.checkIfUserExists(loginData)).thenReturn(false) // Act val result = repo.parseAndLogin(loginData) // Assert - assertEquals(LoginRepository.LoginResult.ExistingAccount, result) + assertTrue(result is LoginRepository.LoginResult.NewAccount) verify(localLoginDataSource).startAccountRemovalWorker() + verify(localLoginDataSource).awaitUserRemoval(any(), any()) + } + + @Test + fun `parseAndLogin reports a pending removal that does not finish in time`() = + runTest { + val loginData = LoginCompletion(200, "https://server.com", "testuser", "apppass123") + whenever(localLoginDataSource.checkIfUserIsScheduledForDeletion(loginData)).thenReturn(true) + whenever(localLoginDataSource.startAccountRemovalWorker()).thenReturn(liveData) + whenever(localLoginDataSource.awaitUserRemoval(any(), any())).thenReturn(false) + + val result = repo.parseAndLogin(loginData) + + assertEquals(LoginRepository.LoginResult.RemovalPending, result) + verify(localLoginDataSource, never()).checkIfUserExists(any()) } @Test @@ -530,6 +548,8 @@ class LoginRepositoryTest { .thenReturn(true) whenever(localLoginDataSource.startAccountRemovalWorker()) .thenReturn(liveData) + whenever(localLoginDataSource.awaitUserRemoval(any(), any())).thenReturn(true) + whenever(localLoginDataSource.checkIfUserExists(loginData)).thenReturn(false) // Act repo.parseAndLogin(loginData)