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 0e18101..05a7bed 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 @@ -9,6 +9,7 @@ import de.jeanlucmakiola.caldav.NextcloudLoginFlow import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.withContext import okhttp3.HttpUrl +import java.util.concurrent.TimeUnit import javax.inject.Inject import javax.inject.Singleton @@ -84,9 +85,19 @@ class OkHttpCalDavGateway @Inject constructor( .getOrNull() } + /** + * ⚠️ The budget lives on the request, as it does for the revocation. + * `execute()` parks on a socket read that no cancellation can break, so the + * four places that cancel the poll job only stop the *next* request — and + * the shared client's ceiling is sized for a multiget, not for a two-second + * poll loop against a server that answered a moment ago. + */ override suspend fun pollLoginFlow(flow: NextcloudLoginFlow.Flow): NextcloudLoginFlow.PollResult = withContext(io) { - NextcloudLoginFlow(CalDavHttp.anonymous(userAgent), userAgent).poll(flow, now()) + val client = CalDavHttp.anonymous(userAgent).newBuilder() + .callTimeout(POLL_TIMEOUT_SECONDS, TimeUnit.SECONDS) + .build() + NextcloudLoginFlow(client, userAgent).poll(flow, now()) } override suspend fun revokeAppPassword( @@ -108,4 +119,9 @@ class OkHttpCalDavGateway @Inject constructor( } private fun now() = System.currentTimeMillis() / 1000 + + private companion object { + /** One poll of a 2s loop. Long enough for a homelab, short enough to cancel. */ + const val POLL_TIMEOUT_SECONDS = 15L + } } 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 8ad1922..af48af0 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 @@ -138,6 +138,18 @@ class AddAccountViewModel @Inject constructor( // *Continue*, not start over — so a second attempt would mint a second // password on top of the first without this. discardMintedPassword() + // ⚠️ And the credential itself, not just the minted one. backToServer is + // reachable *after* a successful approval — a post-approval discovery + // failure lands on an ordinary address step with a live Continue button + // — and only onStartOver cleared these. So: approve on server A, + // discovery fails, type server B, B answers anonymously, and onSave sees + // a non-blank password and creates me@B carrying A's credential. + username = "" + appPassword = "" + // Discovery belongs to the address that produced it, and so does the + // host list the diagnostics read. + found = null + hostsNeedingAuth = emptyList() // The mismatch is sticky on purpose, but it belongs to the address that // produced it — carrying it into a different server's flow makes a claim // about that server's settings which was never measured. @@ -192,33 +204,51 @@ class AddAccountViewModel @Inject constructor( working(AddAccountMessage.Progress.SigningIn) viewModelScope.launch { - val credentials = serverRoot?.let { - CalDavGateway.Credentials(username, appPassword, it) - } + // ⚠️ An error, not a silent unauthenticated retry. serverRootFor + // cannot parse a host-with-path like cloud.example.com/nextcloud, and + // passing null here sent no credentials at all — then reported the + // resulting 401 as "credentials rejected" about a password nothing + // had tried. + val root = serverRoot + ?: return@launch backToServer(CalDavDiscovery.Outcome.Cause.NOT_AN_ADDRESS) + val credentials = CalDavGateway.Credentials(username, appPassword, root) when (val outcome = gateway.discover(typedInput, credentials)) { is CalDavDiscovery.Outcome.Found -> onDiscovered(outcome) // A second rejection is the point at which naming the provider's // own rule is worth more than repeating "wrong password". - is CalDavDiscovery.Outcome.NeedsAuthentication, - CalDavDiscovery.Outcome.Unauthenticated, - -> _state.update { - it.copy( - step = AddAccountStep.EnterCredentials( - username = username, - password = "", - error = quirkHint() ?: crossDomainHint() - ?: AddAccountMessage.CredentialsRejected, - ), - ) + // + // ⚠️ Refreshed here too, not only on the browser path. An + // authenticated PROPFIND reaches further than the anonymous + // probe — principal, then home sets — so the cross-domain home + // set that actually caused this 401 is the one most likely to be + // missing from a list collected before the password was sent, + // and the user gets "credentials rejected" instead of the + // diagnostic that names it. + is CalDavDiscovery.Outcome.NeedsAuthentication -> { + hostsNeedingAuth = outcome.hosts + credentialsRejected() } + CalDavDiscovery.Outcome.Unauthenticated -> credentialsRejected() + is CalDavDiscovery.Outcome.NotCalDav -> backToServer(outcome.cause) is CalDavDiscovery.Outcome.Failed -> backToServer(outcome.cause) } } } + private fun credentialsRejected() = _state.update { + it.copy( + step = AddAccountStep.EnterCredentials( + username = username, + password = "", + error = quirkHint() ?: crossDomainHint() + ?: AddAccountMessage.CredentialsRejected, + ), + ) + } + /** Step 2b → the browser flow, if this looks like a Nextcloud. */ private suspend fun offerSignIn() { val root = serverRoot ?: return backToServer(CalDavDiscovery.Outcome.Cause.NOT_AN_ADDRESS) 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 c8c4bc1..57bc2f7 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 @@ -418,6 +418,60 @@ class AddAccountViewModelTest { } } + @Nested + inner class TheCredentialsStep { + + @Test + fun `an address with no parseable root is an error, not a silent retry`() = + runTest(dispatcher) { + // serverRootFor cannot parse a host-with-path, and passing null + // sent no credentials at all — then reported the 401 as + // "credentials rejected" about a password nothing had tried. + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = null + + val vm = viewModel() + vm.onServerInputChanged("cloud.example.com/nextcloud") + vm.onServerSubmitted() + advanceUntilIdle() + vm.onUsernameChanged("me") + vm.onPasswordChanged("pw") + vm.onCredentialsSubmitted() + advanceUntilIdle() + + val step = vm.state.value.step as AddAccountStep.EnterServer + assertThat(step.error).isEqualTo(CalDavDiscovery.Outcome.Cause.NOT_AN_ADDRESS) + // One discovery, the anonymous one. Nothing was sent blind. + assertThat(gateway.discoveries).hasSize(1) + } + + @Test + fun `the authenticated pass refreshes the hosts the diagnostic reads`() = + runTest(dispatcher) { + // The anonymous probe stops at the root; an authenticated one + // reaches the principal and its home sets, so the cross-domain + // home set behind the 401 first appears here. + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = null + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication( + listOf("dav.elsewhere.test"), + ) + + val vm = viewModel() + vm.onServerInputChanged("https://cloud.example.com/") + vm.onServerSubmitted() + advanceUntilIdle() + vm.onUsernameChanged("me") + vm.onPasswordChanged("pw") + vm.onCredentialsSubmitted() + advanceUntilIdle() + + val step = vm.state.value.step as AddAccountStep.EnterCredentials + assertThat(step.error) + .isEqualTo(AddAccountMessage.OutsideCredentialScope("dav.elsewhere.test")) + } + } + @Nested inner class ChoosingLists { @@ -484,6 +538,47 @@ class AddAccountViewModelTest { .isInstanceOf(AddAccountStep.EnterCredentials::class.java) } + @Test + fun `a new address does not inherit the previous server's credentials`() = + runTest(dispatcher) { + // Approve on A, discovery fails afterwards, type B — which + // answers anonymously. The screen offers *Continue* here, not + // start over, so nothing else clears what A minted. + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication( + listOf("cloud.example.com"), + ) + gateway.loginFlow = flow() + gateway.pollResults += NextcloudLoginFlow.PollResult.Approved( + NextcloudLoginFlow.Credentials( + server = "https://cloud.example.com/".toHttpUrl(), + loginName = "me", + appPassword = "a-pw", + ), + ) + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.Failed( + CalDavDiscovery.Outcome.Cause.UNREACHABLE, + "no route to host", + ) + val vm = viewModel() + vm.onServerInputChanged("https://cloud.example.com/") + vm.onServerSubmitted() + advanceUntilIdle() + + gateway.loginFlow = null + gateway.discoveryOutcomes += found(collection("Tasks")) + vm.onServerInputChanged("https://other.example.com/") + vm.onServerSubmitted() + advanceUntilIdle() + vm.onSave() + advanceUntilIdle() + + // ⚠️ Otherwise an account named me@other.example.com is created + // carrying cloud.example.com's app password. + assertThat(creator.created).isEmpty() + assertThat(vm.state.value.step) + .isInstanceOf(AddAccountStep.EnterCredentials::class.java) + } + @Test fun `a failure inside create leaves a translatable message, not a spinner`() = runTest(dispatcher) {