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 f83702a..dc59e41 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 @@ -13,6 +13,8 @@ import de.jeanlucmakiola.caldav.ServiceDiscovery import de.jeanlucmakiola.caldav.TaskCollection import de.jeanlucmakiola.agendula.data.di.ApplicationScope import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.NonCancellable +import kotlinx.coroutines.withContext import kotlinx.coroutines.Job import kotlinx.coroutines.delay import kotlinx.coroutines.flow.MutableStateFlow @@ -131,6 +133,11 @@ class AddAccountViewModel @Inject constructor( fun onServerSubmitted() { val input = (_state.value.step as? AddAccountStep.EnterServer)?.input?.trim().orEmpty() if (input.isEmpty()) return + // ⚠️ The retry path, and the one that actually leaks. A post-approval + // failure sends the user back here with an error, and the screen offers + // *Continue*, not start over — so a second attempt would mint a second + // password on top of the first without this. + discardMintedPassword() typedInput = input val quirk = ServerQuirk.forInput(input) @@ -242,15 +249,23 @@ class AddAccountViewModel @Inject constructor( delay(POLL_INTERVAL_MILLIS) when (val result = gateway.pollLoginFlow(flow)) { is NextcloudLoginFlow.PollResult.Approved -> { - 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, - ) + // ⚠️ Uncancellable. The 200 that carried this password is + // the only one there will ever be — the server deletes the + // flow row before answering — so a cancellation delivered + // between that answer and the assignment below spends the + // credential with nothing left holding it. Recording it is + // what makes handing it back possible. + withContext(NonCancellable) { + 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. @@ -371,10 +386,17 @@ class AddAccountViewModel @Inject constructor( } when (outcome) { is AccountRepository.Outcome.Created -> { - // Ownership passes to the account, which revokes on removal. + // Ownership passes to the account, which revokes on removal — + // but only if this is the password it was saved with. Every + // route from a minted password to a typed one already + // discards on the way, so the else branch should be dead; + // it is here because the failure it guards is silent and + // delayed. The account syncs once and then 401s for ever, + // long after anyone would connect it to adding it. + // // Cleared before the state update: an exception there must not // leave a saved account's own credential queued for revoking. - minted = null + if (minted?.password == appPassword) minted = null else discardMintedPassword() _state.update { it.copy(step = AddAccountStep.Done) } } @@ -429,7 +451,9 @@ class AddAccountViewModel @Inject constructor( private fun discardMintedPassword() { val credentials = minted ?: return minted = null - appScope.launch { gateway.revokeIssuedAppPassword(credentials) } + // The scope has no exception handler, so anything escaping here takes the + // app down — for a courtesy call whose failures are deliberately silent. + appScope.launch { runCatching { gateway.revokeIssuedAppPassword(credentials) } } } // ------------------------------------------------------------- internals @@ -520,7 +544,12 @@ class AddAccountViewModel @Inject constructor( val root = serverRoot ?: return null val scope = root.topPrivateDomain() ?: root.host return hosts.firstOrNull { host -> - val candidate = "https://$host".toHttpUrlOrNull() ?: return@firstOrNull true + // OkHttp hands back IPv6 literals unbracketed, which will not parse + // again. Unparseable reads as in-scope: the cost of a diagnostic that + // stays quiet is nothing, and the caller now treats out-of-scope as + // fatal — so failing the other way strands a homelab on [::1]. + val literal = if (':' in host) "[$host]" else host + val candidate = "https://$literal".toHttpUrlOrNull() ?: return@firstOrNull false !(candidate.topPrivateDomain() ?: candidate.host).equals(scope, ignoreCase = true) } } 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 135b228..e98424b 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 @@ -291,7 +291,15 @@ class AddAccountViewModelTest { ), ) - vm.onStartOver() + // ⚠️ The path the user actually has. After a post-approval failure + // the screen shows the address with an error and a *Continue* button + // — there is no start-over here, so a test that used one would pass + // against an affordance nobody can reach. + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.Failed( + CalDavDiscovery.Outcome.Cause.UNREACHABLE, + "no route to host", + ) + vm.onServerSubmitted() advanceUntilIdle() // Nextcloud hands it over exactly once. Walking away leaves it valid @@ -345,6 +353,25 @@ class AddAccountViewModelTest { assertThat(gateway.revoked).isEmpty() } + @Test + fun `a typed password after an approval hands the minted one back`() = + runTest(dispatcher) { + val vm = approveThen( + CalDavDiscovery.Outcome.NeedsAuthentication(listOf("cloud.example.com")), + ) + // Retry, this time with no login flow on offer, so the user types + // their own password instead. + gateway.loginFlow = null + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication( + listOf("cloud.example.com"), + ) + vm.onServerSubmitted() + advanceUntilIdle() + + // The retry itself hands back the first password. + assertThat(gateway.revoked.map { it.password }).containsExactly("app-pw") + } + @Test fun `a revoke that fails is never surfaced`() = runTest(dispatcher) { gateway.revokeSucceeds = false