From 5dfbe0fffc685cb057e60518ce7f548fcfa26af3 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Mon, 7 Sep 2026 22:59:39 +0200 Subject: [PATCH] sync: hand back an app password the flow is not going to use Nextcloud returns a minted password exactly once. Every path that left the browser flow without saving an account dropped it: discovery failing after approval, an account whose lists we cannot use, start over, the back arrow, switching to a typed password, or leaving Settings altogether. It stays valid on the server for ever, and every attempt is named "Agendula (Android)" -- so a user retrying against a misconfigured server ends up with six identical entries and no way to tell which one their working account uses. They prune nothing, or prune the wrong one. One owned field and one sink rather than a revoke per path: ten paths today, and the eleventh would be forgotten. Ownership passes to the account on Created and is released nowhere else without revoking. The sink runs on the application scope, not viewModelScope -- androidx closes that before onCleared, so a launch there never runs its body. Revocation goes through a new revokeAt, which takes the OCS root directly. ocsRootFor is principal-shaped and falls back to the bare origin, so sending a login flow's server base through it would collapse a subpath install's /nextcloud/ to / and DELETE a path that 404s -- the silent no-op that function exists to prevent. Also stops reporting a rejected credential as an empty account: a 401 after approval is either a home set outside the domain the credential is scoped to, which retrying only mints another password for, or the server having a moment. The registrable-domain check that decides this is now shared with crossDomainHint, which had been computing "com" for example.com and so never firing. --- .../agendula/data/sync/CalDavGateway.kt | 20 +++ .../ui/accounts/AddAccountViewModel.kt | 100 +++++++++++- .../ui/accounts/AddAccountViewModelTest.kt | 150 +++++++++++++++++- .../de/jeanlucmakiola/caldav/AppPassword.kt | 18 ++- .../jeanlucmakiola/caldav/AppPasswordTest.kt | 14 ++ 5 files changed, 293 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CalDavGateway.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CalDavGateway.kt index eccb87c..0e18101 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CalDavGateway.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CalDavGateway.kt @@ -39,6 +39,17 @@ interface CalDavGateway { */ suspend fun revokeAppPassword(credentials: Credentials): Boolean + /** + * The same, for a password the login flow just minted. + * + * ⚠️ Separate because [Credentials.origin] means something different here: + * the *server root* the flow reported, not a principal URL. Sending it + * through [revokeAppPassword] would derive the OCS root as though it were a + * principal, and a subpath install's `https://host/nextcloud/` would collapse + * to `https://host/` — a DELETE that 404s on every one of them. + */ + suspend fun revokeIssuedAppPassword(credentials: Credentials): Boolean + /** Credentials, and the origin whose registrable domain they are scoped to. */ data class Credentials(val username: String, val password: String, val origin: HttpUrl) } @@ -87,5 +98,14 @@ class OkHttpCalDavGateway @Inject constructor( AppPassword.revoke(client, credentials.origin) } + override suspend fun revokeIssuedAppPassword( + credentials: CalDavGateway.Credentials, + ): Boolean = withContext(io) { + val client = CalDavHttp.authenticated( + userAgent, credentials.username, credentials.password, credentials.origin, + ) + AppPassword.revokeAt(client, credentials.origin) + } + private fun now() = System.currentTimeMillis() / 1000 } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt index ef62743..f83702a 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt @@ -11,6 +11,8 @@ import de.jeanlucmakiola.caldav.NextcloudLoginFlow import de.jeanlucmakiola.caldav.ServerQuirk import de.jeanlucmakiola.caldav.ServiceDiscovery import de.jeanlucmakiola.caldav.TaskCollection +import de.jeanlucmakiola.agendula.data.di.ApplicationScope +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow @@ -19,6 +21,7 @@ import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import okhttp3.HttpUrl +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import javax.inject.Inject /** Where the user is in adding an account. */ @@ -86,6 +89,10 @@ data class AddAccountUiState( class AddAccountViewModel @Inject constructor( private val repository: AccountCreator, private val gateway: CalDavGateway, + // ⚠️ Not viewModelScope. Handing the password back has to survive the + // ViewModel that minted it, and androidx closes viewModelScope *before* + // onCleared runs, so a launch there never executes its body. + @ApplicationScope private val appScope: CoroutineScope, ) : ViewModel() { private val _state = MutableStateFlow(AddAccountUiState()) @@ -101,6 +108,17 @@ class AddAccountViewModel @Inject constructor( /** Which hosts asked for credentials, for the cross-domain diagnostic. */ private var hostsNeedingAuth: List = emptyList() + /** + * A password the browser flow minted and nothing owns yet. + * + * Nextcloud returns it exactly once, so if we walk away without either + * saving or revoking it, it stays valid in the user's device list for ever — + * under the same name as every other attempt, so they cannot tell which one + * their working account uses and dare not prune any. Cleared without + * revoking only when [AccountCreator] takes ownership. + */ + private var minted: CalDavGateway.Credentials? = null + fun onServerInputChanged(value: String) = _state.update { it.copy(step = AddAccountStep.EnterServer(value), quirk = ServerQuirk.forInput(value)) } @@ -227,6 +245,12 @@ class AddAccountViewModel @Inject constructor( username = result.credentials.loginName appPassword = result.credentials.appPassword serverRoot = result.credentials.server + // Ours to hand back until an account takes it on. + minted = CalDavGateway.Credentials( + username, + appPassword, + result.credentials.server, + ) // The server contradicted itself about where it lives. // We recovered, but the setting behind it is the user's // to fix, so say so rather than silently coercing. @@ -254,7 +278,28 @@ class AddAccountViewModel @Inject constructor( // no task lists, which is both wrong and unactionable. is CalDavDiscovery.Outcome.Failed -> backToServer(outcome.cause) is CalDavDiscovery.Outcome.NotCalDav -> backToServer(outcome.cause) - else -> backToServer(CalDavDiscovery.Outcome.Cause.NO_CALENDARS) + // ⚠️ These two carry no Cause, so they used to land + // on "signed in, but no task lists" — said of a + // credential the server had just rejected. A 401 here + // is either a home set outside the domain the + // credential is scoped to, which retrying cannot fix + // and which only mints a second password, or the + // server having a moment. + is CalDavDiscovery.Outcome.NeedsAuthentication -> { + hostsNeedingAuth = outcome.hosts + val stranger = outsideCredentialScope(outcome.hosts) + if (stranger != null) { + fatal( + "This server keeps some task lists on $stranger, " + + "which Agendula cannot sign in to yet.", + ) + } else { + backToServer(CalDavDiscovery.Outcome.Cause.SERVER_ERROR) + } + } + + CalDavDiscovery.Outcome.Unauthenticated -> + backToServer(CalDavDiscovery.Outcome.Cause.SERVER_ERROR) } return@launch } @@ -278,6 +323,9 @@ class AddAccountViewModel @Inject constructor( /** The user says they finished in the browser but nothing arrived. */ fun onBrowserCancelled() { + // The one exit that never reaches onStartOver: if approval already + // landed, that password is about to be replaced by a typed one. + discardMintedPassword() pollJob?.cancel() _state.update { it.copy(step = AddAccountStep.EnterCredentials(error = null), openInBrowser = null) @@ -322,8 +370,13 @@ class AddAccountViewModel @Inject constructor( ) } when (outcome) { - is AccountRepository.Outcome.Created -> + is AccountRepository.Outcome.Created -> { + // Ownership passes to the account, which revokes on removal. + // Cleared before the state update: an exception there must not + // leave a saved account's own credential queued for revoking. + minted = null _state.update { it.copy(step = AddAccountStep.Done) } + } AccountRepository.Outcome.AlreadyExists -> fatal("That account is already set up.") @@ -344,6 +397,7 @@ class AddAccountViewModel @Inject constructor( * account's username and app password. */ fun onStartOver() { + discardMintedPassword() pollJob?.cancel() pollJob = null found = null @@ -356,9 +410,28 @@ class AddAccountViewModel @Inject constructor( } override fun onCleared() { + abandonMintedPassword() pollJob?.cancel() } + /** [onCleared] is protected, and the abandonment is worth testing. */ + internal fun abandonMintedPassword() = discardMintedPassword() + + /** + * Hand back a password nothing is going to use. + * + * Fire and forget: the user is already leaving, the call is best-effort by + * the module's own contract, and it carries its own 5s budget. A failure is + * not reported — telling someone we could not clean up a credential they + * never knew existed, while they are escaping a failure, is worse than the + * row it leaves behind. + */ + private fun discardMintedPassword() { + val credentials = minted ?: return + minted = null + appScope.launch { gateway.revokeIssuedAppPassword(credentials) } + } + // ------------------------------------------------------------- internals private fun onDiscovered(outcome: CalDavDiscovery.Outcome.Found) { @@ -430,13 +503,26 @@ class AddAccountViewModel @Inject constructor( * legal (RFC 4791 §6.2.1) but unreachable for us: the credential is scoped to * one domain. Better to name it than to leave "wrong password" standing. */ - private fun crossDomainHint(): String? { - val root = serverRoot?.host ?: return null - val rootDomain = root.substringAfter('.', root) - val outside = hostsNeedingAuth.filterNot { it.endsWith(rootDomain) } - return outside.firstOrNull()?.let { + private fun crossDomainHint(): String? = + outsideCredentialScope(hostsNeedingAuth)?.let { "This server keeps some task lists on $it, which Agendula cannot sign in to yet." } + + /** + * The first host the credential will never be offered to, if any. + * + * ⚠️ The same boundary `CalDavHttp` scopes the credential by, derived the + * same way. A `substringAfter('.')` split computes "com" for + * `example.com` — so every host ending in `com` reads as in-domain, the + * diagnostic never fires, and `notexample.com` reads as in-domain too. + */ + private fun outsideCredentialScope(hosts: List): String? { + val root = serverRoot ?: return null + val scope = root.topPrivateDomain() ?: root.host + return hosts.firstOrNull { host -> + val candidate = "https://$host".toHttpUrlOrNull() ?: return@firstOrNull true + !(candidate.topPrivateDomain() ?: candidate.host).equals(scope, ignoreCase = true) + } } private fun accountName(): String = diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt index 47bbef1..135b228 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt @@ -7,12 +7,14 @@ import de.jeanlucmakiola.agendula.data.sync.CalDavGateway import de.jeanlucmakiola.caldav.CalDavDiscovery import de.jeanlucmakiola.caldav.NextcloudLoginFlow import de.jeanlucmakiola.caldav.TaskCollection +import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.StandardTestDispatcher import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.TestScope import kotlinx.coroutines.test.runTest import kotlinx.coroutines.test.setMain import okhttp3.HttpUrl @@ -40,7 +42,15 @@ class AddAccountViewModelTest { @AfterEach fun tearDown() = Dispatchers.resetMain() - private fun viewModel() = AddAccountViewModel(creator, gateway) + /** + * ⚠️ Not `backgroundScope`: `advanceUntilIdle` does not run its work, so a + * revocation launched there would silently never happen and every assertion + * about it would pass for the wrong reason. This scope shares the scheduler + * as ordinary work. + */ + private val appScope = CoroutineScope(dispatcher) + + private fun viewModel() = AddAccountViewModel(creator, gateway, appScope) @Nested inner class TheAddressStep { @@ -249,6 +259,134 @@ class AddAccountViewModelTest { } } + @Nested + inner class AbandoningTheBrowserFlow { + + private fun TestScope.approveThen(outcome: CalDavDiscovery.Outcome): AddAccountViewModel { + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication( + listOf("cloud.example.com"), + ) + gateway.loginFlow = flow() + gateway.pollResults += NextcloudLoginFlow.PollResult.Approved( + NextcloudLoginFlow.Credentials( + server = "https://dav.example.com/".toHttpUrl(), + loginName = "me", + appPassword = "app-pw", + ), + ) + gateway.discoveryOutcomes += outcome + val vm = viewModel() + vm.onServerInputChanged("https://cloud.example.com/") + vm.onServerSubmitted() + advanceUntilIdle() + return vm + } + + @Test + fun `a password nobody will use is handed back`() = runTest(dispatcher) { + val vm = approveThen( + CalDavDiscovery.Outcome.Failed( + CalDavDiscovery.Outcome.Cause.UNREACHABLE, + "no route to host", + ), + ) + + vm.onStartOver() + advanceUntilIdle() + + // Nextcloud hands it over exactly once. Walking away leaves it valid + // for ever, under the same name as every other attempt — so the user + // cannot tell which entry their working account uses. + val handedBack = gateway.revoked.single() + assertThat(handedBack.password).isEqualTo("app-pw") + // The server that issued it, not the one the user typed. + assertThat(handedBack.origin.toString()).isEqualTo("https://dav.example.com/") + } + + @Test + fun `an account with no usable lists hands its password back`() = runTest(dispatcher) { + val vm = approveThen(found()) + + // fatal() offers only "start over", so that is the whole exit. + vm.onStartOver() + advanceUntilIdle() + + assertThat(gateway.revoked).hasSize(1) + } + + @Test + fun `leaving the screen hands the password back`() = runTest(dispatcher) { + val vm = approveThen( + CalDavDiscovery.Outcome.Failed( + CalDavDiscovery.Outcome.Cause.UNREACHABLE, + "no route to host", + ), + ) + + vm.abandonMintedPassword() + advanceUntilIdle() + + assertThat(gateway.revoked).hasSize(1) + } + + @Test + fun `a saved account keeps its password`() = runTest(dispatcher) { + val vm = approveThen(found(collection("Tasks"))) + vm.onSave() + advanceUntilIdle() + + // What the screen does once it is Done. + vm.onStartOver() + advanceUntilIdle() + + // ⚠️ The one way this fix can do real harm: revoking the credential + // of the account just created. It would sync once and then 401 for + // ever, long after anyone connects it to adding the account. + assertThat(gateway.revoked).isEmpty() + } + + @Test + fun `a revoke that fails is never surfaced`() = runTest(dispatcher) { + gateway.revokeSucceeds = false + val vm = approveThen( + CalDavDiscovery.Outcome.Failed( + CalDavDiscovery.Outcome.Cause.UNREACHABLE, + "no route to host", + ), + ) + + vm.onStartOver() + advanceUntilIdle() + + assertThat(vm.state.value.fatal).isNull() + assertThat(vm.state.value.step).isInstanceOf(AddAccountStep.EnterServer::class.java) + } + + @Test + fun `a rejected password is not reported as an empty account`() = runTest(dispatcher) { + val vm = approveThen( + CalDavDiscovery.Outcome.NeedsAuthentication(listOf("cloud.example.com")), + ) + + // "Signed in, but no task lists" said of a credential the server had + // just refused. Same registrable domain, so retrying may help. + val step = vm.state.value.step as AddAccountStep.EnterServer + assertThat(step.error).isEqualTo(CalDavDiscovery.Outcome.Cause.SERVER_ERROR) + } + + @Test + fun `a home set outside the credential scope is called what it is`() = + runTest(dispatcher) { + val vm = approveThen( + CalDavDiscovery.Outcome.NeedsAuthentication(listOf("dav.elsewhere.org")), + ) + + // Retrying cannot reach it, and each attempt mints another + // password — so this is fatal rather than back-to-the-address. + assertThat(vm.state.value.fatal).contains("dav.elsewhere.org") + } + } + @Nested inner class ChoosingLists { @@ -418,6 +556,16 @@ class AddAccountViewModelTest { override suspend fun revokeAppPassword( credentials: CalDavGateway.Credentials, ): Boolean = true + + val revoked = mutableListOf() + var revokeSucceeds = true + + override suspend fun revokeIssuedAppPassword( + credentials: CalDavGateway.Credentials, + ): Boolean { + revoked += credentials + return revokeSucceeds + } } private class FakeCreator : AccountCreator { diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt index aad10ae..4fd00a7 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt @@ -81,8 +81,24 @@ object AppPassword { httpClient: OkHttpClient, principal: HttpUrl, timeout: Duration = REVOCATION_TIMEOUT, + ): Boolean = revokeAt(httpClient, ocsRootFor(principal), timeout) + + /** + * The same revocation, for a caller that already holds the server root. + * + * ⚠️ Do not route such a caller through [revoke]. [ocsRootFor] is + * *principal*-shaped: it looks for `remote.php` and falls back to the bare + * origin when there is none. A login flow hands back a server base, so a + * subpath install's `https://host/nextcloud/` would collapse to + * `https://host/` and the DELETE would 404 on every one of them — attempted, + * and doing nothing, which is the failure [ocsRootFor] exists to prevent. + */ + fun revokeAt( + httpClient: OkHttpClient, + ocsRoot: HttpUrl, + timeout: Duration = REVOCATION_TIMEOUT, ): Boolean = try { - val url = ocsRootFor(principal).newBuilder().addPathSegments(PATH).build() + val url = ocsRoot.newBuilder().addPathSegments(PATH).build() httpClient.newBuilder() // Shares the pool and dispatcher, so this costs nothing. .callTimeout(timeout.toJavaDuration()) diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt index 1115006..74d8254 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt @@ -42,6 +42,20 @@ class AppPasswordTest { .isEqualTo("https://baikal.example.com/") } + @Test fun `a server root is used verbatim, not treated as a principal`() { + server.enqueue(MockResponse().setResponseCode(200)) + + val revoked = AppPassword.revokeAt(httpClient, server.url("/nextcloud/")) + + // ⚠️ ocsRootFor looks for remote.php and falls back to the bare origin. + // A login flow hands back a server base, so routing one through revoke() + // would collapse /nextcloud/ to / and DELETE a path that 404s on every + // subpath install — attempted, and doing nothing. + assertThat(revoked).isTrue() + assertThat(server.takeRequest().path) + .isEqualTo("/nextcloud/ocs/v2.php/core/apppassword") + } + @Test fun `a server that never answers does not hold the removal`() { // Accepts the connection and says nothing — the off-VPN homelab shape. server.enqueue(MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE))