diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountScreen.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountScreen.kt index b5feda4..8d7256a 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountScreen.kt @@ -162,6 +162,19 @@ internal fun AddAccountScreen( is AddAccountStep.ChooseLists -> ListsStep(step, viewModel) AddAccountStep.Done -> Unit } + + // Learned during the browser step and shown from there on, whichever + // step follows: the address it blames is one the user has to go and + // correct on the server, and it is just as true once the lists load. + state.originMismatch?.let { mismatch -> + QuirkNote( + text = stringResource( + R.string.add_account_origin_mismatch, + mismatch.actual, + mismatch.expected, + ), + ) + } } } 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 d70529d..ef62743 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 @@ -67,6 +67,15 @@ data class AddAccountUiState( * login flow to a different host than the one typed. */ val quirk: ServerQuirk? = null, + /** + * The login flow named an origin we could not have reached, and we used the + * one that answered instead. + * + * Sticky, unlike the per-step note: it is learned during the browser step + * but the setting it blames is what the user has to go and fix, so it has to + * survive into the steps after it. + */ + val originMismatch: NextcloudLoginFlow.HostMismatch? = null, /** Set when the flow cannot continue at all; the UI offers only "start over". */ val fatal: String? = null, /** Non-null once the browser flow has a URL to open. */ @@ -218,6 +227,12 @@ class AddAccountViewModel @Inject constructor( username = result.credentials.loginName appPassword = result.credentials.appPassword serverRoot = 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. + result.hostMismatch?.let { mismatch -> + _state.update { it.copy(originMismatch = mismatch) } + } working("Reading your task lists") val outcome = gateway.discover( // The server the credentials were *issued by*, not the diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index bc573ba..a3da8ab 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -355,6 +355,7 @@ Open the browser again Use a password instead The server sent us to %1$s, but you typed %2$s. That usually means its overwrite.cli.url setting is wrong. + The server reported its address as %1$s, which this device cannot reach, so %2$s was used instead. Its overwrite.cli.url or trusted_proxies setting is probably wrong. Which lists should sync? Read-only 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 cddc448..47bbef1 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 @@ -136,6 +136,37 @@ class AddAccountViewModelTest { .isEqualTo("https://dav.example.com/") } + @Test + fun `an origin the server got wrong is surfaced and stays visible`() = + runTest(dispatcher) { + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication( + listOf("cloud.example.com"), + ) + gateway.loginFlow = flow() + gateway.pollResults += NextcloudLoginFlow.PollResult.Approved( + credentials = NextcloudLoginFlow.Credentials( + server = "https://cloud.example.com/".toHttpUrl(), + loginName = "me", + appPassword = "app-pw", + ), + hostMismatch = NextcloudLoginFlow.HostMismatch( + expected = "cloud.example.com", + actual = "nextcloud", + ), + ) + gateway.discoveryOutcomes += found(collection("Tasks")) + + val vm = viewModel() + vm.onServerInputChanged("https://cloud.example.com/") + vm.onServerSubmitted() + advanceUntilIdle() + + // Sticky: it is learned mid-flow, but the setting it blames is + // what the user has to go and fix afterwards. + assertThat(vm.state.value.step).isInstanceOf(AddAccountStep.ChooseLists::class.java) + assertThat(vm.state.value.originMismatch?.actual).isEqualTo("nextcloud") + } + @Test fun `a discovery failure after approval reports what actually failed`() = runTest(dispatcher) { diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt index 5e1c2ca..e58d1c0 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt @@ -79,7 +79,7 @@ class NextcloudLoginFlow( */ fun start(server: HttpUrl, now: Long): Result = runCatching { val request = Request.Builder() - .url(server.newBuilder().addPathSegments("index.php/login/v2").build()) + .url(server.newBuilder().addPathSegments(FLOW_START_PATH).build()) .header("User-Agent", userAgent) // OCS-APIRequest is *not* needed: v2 is a Frontpage route, not OCS. .post(FormBody.Builder().build()) @@ -243,10 +243,14 @@ class NextcloudLoginFlow( * and the app password is already spent. * * So when the claimed origin is outside the registrable domain we just - * successfully polled, keep scheme, host and port from the poll endpoint — - * the origin empirically known to answer — and keep the claimed path. - * Coerced, never refused: by this point the credential exists and the 200 is - * spent, so throwing burns a live app password. + * successfully polled, rebuild it entirely from the poll endpoint — scheme, + * host, port *and* base path — which is the one URL empirically known to + * answer. The claimed path is dropped with the claimed host: both come from + * the same generator, and keeping half of a URL we have decided not to trust + * is how a subdirectory install loses its prefix. The endpoint's own prefix + * is whatever precedes `index.php/login/v2/poll`, which is where the flow + * was opened. Coerced, never refused: by this point the credential exists + * and the 200 is spent, so throwing burns a live app password. * * This compares one server-emitted origin against another, never against * what the user typed, so a correctly configured proxy — which emits the @@ -262,12 +266,26 @@ class NextcloudLoginFlow( val polled = expected.topPrivateDomain() ?: expected.host if (claimed.equals(polled, ignoreCase = true)) return secure return expected.newBuilder() - .encodedPath(secure.encodedPath) + .encodedPath(baseOf(expected)) .query(null) .fragment(null) .build() } + private companion object { + const val FLOW_START_PATH = "index.php/login/v2" + + /** What `start()` appends, plus the poll leg the server adds to it. */ + const val FLOW_PATH = "index.php/login/v2/poll" + } + + /** The poll endpoint with the flow's own path removed — the server's web root. */ + private fun baseOf(pollEndpoint: HttpUrl): String { + val path = pollEndpoint.encodedPath + val tail = path.indexOf(FLOW_PATH) + return if (tail >= 0) path.take(tail).ifEmpty { "/" } else "/" + } + /** A different host than the user typed — reported, not refused. */ internal fun hostMismatchOf(expected: HttpUrl, actual: HttpUrl): HostMismatch? = if (expected.host != actual.host) HostMismatch(expected.host, actual.host) else null diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt index 5ce8ab4..5ca7b2d 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt @@ -233,15 +233,30 @@ class NextcloudLoginFlowTest { assertThat(flow.reachableOrigin(expected, actual)).isEqualTo(actual) } - @Test fun `the claimed path survives the host being replaced`() { + @Test fun `a subdirectory install keeps its prefix when the host is replaced`() { + val flow = NextcloudLoginFlow(OkHttpClient(), "test") + val expected = + "https://cloud.example.com/nextcloud/index.php/login/v2/poll".toHttpUrl() + + // ⚠️ The container's own webroot is empty, so the claimed URL carries no + // prefix at all. Keeping the claimed path alongside the polled host would + // drop /nextcloud and send discovery to the wrong base — with the app + // password already spent. + val reachable = flow.reachableOrigin(expected, "http://nextcloud:11000/".toHttpUrl()) + + assertThat(reachable.host).isEqualTo("cloud.example.com") + assertThat(reachable.encodedPath).isEqualTo("/nextcloud/") + } + + @Test fun `a webroot install is rebuilt at the root`() { val flow = NextcloudLoginFlow(OkHttpClient(), "test") val expected = "https://cloud.example.com/index.php/login/v2/poll".toHttpUrl() - val reachable = flow.reachableOrigin(expected, "https://internal.local/nextcloud/".toHttpUrl()) + val reachable = flow.reachableOrigin(expected, "http://nextcloud:11000/whatever".toHttpUrl()) - // A subdirectory install still lives under its subdirectory. - assertThat(reachable.host).isEqualTo("cloud.example.com") - assertThat(reachable.encodedPath).isEqualTo("/nextcloud/") + // The claimed path goes with the claimed host: both came from the + // generator we just decided not to trust. + assertThat(reachable.encodedPath).isEqualTo("/") } @Test fun `a single-label host is compared exactly, not by a null domain`() {