sync: tell the user which address their server got wrong
Two gaps left by fc7e520, both found in review.
The claimed path was kept when the claimed host was replaced, though
both come from the generator we had just decided not to trust. A
subdirectory install behind a proxy reports an empty webroot from inside
the container, so /nextcloud was dropped and discovery ran against the
wrong base -- the same dead end, one level down. The origin is now
rebuilt entirely from the poll endpoint, whose own prefix is whatever
precedes index.php/login/v2/poll.
And the mismatch rode out of poll() with nowhere to go. It is carried on
the state rather than the step, because it is learned during the browser
step while the setting it blames is what the user has to go and fix
afterwards -- so it has to outlive the step that discovered it.
This commit is contained in:
@@ -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,
|
||||
),
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -355,6 +355,7 @@
|
||||
<string name="add_account_browser_reopen">Open the browser again</string>
|
||||
<string name="add_account_browser_use_password">Use a password instead</string>
|
||||
<string name="add_account_browser_host_mismatch">The server sent us to %1$s, but you typed %2$s. That usually means its overwrite.cli.url setting is wrong.</string>
|
||||
<string name="add_account_origin_mismatch">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.</string>
|
||||
|
||||
<string name="add_account_lists_title">Which lists should sync?</string>
|
||||
<string name="add_account_lists_read_only">Read-only</string>
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -79,7 +79,7 @@ class NextcloudLoginFlow(
|
||||
*/
|
||||
fun start(server: HttpUrl, now: Long): Result<Flow> = 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
|
||||
|
||||
@@ -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`() {
|
||||
|
||||
Reference in New Issue
Block a user