sync(chunk 2d): the account-add flow
One flow, one back-stack entry, as a stepper rather than four destinations —
the steps are not independently reachable, and "back" from the browser step
abandons a server-side flow rather than popping a screen. It hangs off Settings
with the same sliding-section pattern Storage -> Export uses.
address -> discovery -> (Nextcloud browser approval | username + password)
-> pick lists -> add account
Provider warnings come before the attempt, not after: type a Fastmail or iCloud
address and the app password rule is stated while you type, which is the single
most common support ticket a CalDAV client inherits. Google is refused with the
reason. A login-flow host mismatch is shown, not refused — reverse proxies are
ordinary on self-hosted installs.
PreemptiveBasicInterceptor is deleted. The vendored BasicDigestAuthHandler
already sends Basic preemptively over HTTPS, also does Digest (Baikal defaults
to it, OkHttp has none), caches the working scheme, and scopes by registrable
domain — which is what iCloud's cross-host home set needs. Two implementations
of one job is the defect chunk 1 removed from ICalendarWriter.
⚠️ That handler compares its `domain` against the *registrable* domain, so
passing the full host meant credentials were withheld from every request to
every subdomain — i.e. every self-hosted Nextcloud, silently 401ing forever.
Pinned by CalDavHttpTest.
CalDavGateway and AccountCreator put the network and the database behind
interfaces so the sign-in state machine is testable without a server, a
database, a Keystore or an AccountManager. It had no tests, and the review
found eight issues in it.
Account creation is transactional and rolls its lists back explicitly:
account_id is ON DELETE SET NULL, so deleting the row alone leaves orphan
lists behind and every retry adds another set.
This commit is contained in:
@@ -60,6 +60,8 @@ class CalDavDiscovery(
|
||||
data class Found(
|
||||
val principal: HttpUrl,
|
||||
val collections: List<TaskCollection>,
|
||||
/** Every `calendar-home-set` href, in the order the principal listed them. */
|
||||
val homeSets: List<HttpUrl>,
|
||||
/** Set when a 301/308 moved us; the caller must persist it. */
|
||||
val movedTo: HttpUrl?,
|
||||
/** Home sets on a different host than the principal. Normative, but worth surfacing. */
|
||||
@@ -244,6 +246,7 @@ class CalDavDiscovery(
|
||||
return Outcome.Found(
|
||||
principal = principal,
|
||||
collections = collections.values.toList(),
|
||||
homeSets = homeSets.distinct(),
|
||||
movedTo = movedTo,
|
||||
crossHostHomeSets = crossHost,
|
||||
failedHomeSets = failures,
|
||||
|
||||
@@ -0,0 +1,89 @@
|
||||
package de.jeanlucmakiola.caldav
|
||||
|
||||
import at.bitfire.dav4jvm.BasicDigestAuthHandler
|
||||
import at.bitfire.dav4jvm.UrlUtils
|
||||
import okhttp3.HttpUrl
|
||||
import okhttp3.OkHttpClient
|
||||
import java.util.concurrent.TimeUnit
|
||||
|
||||
/**
|
||||
* The HTTP clients the CalDAV layer talks through.
|
||||
*
|
||||
* ⚠️ `followRedirects(false)` is mandatory, not a preference: `DavResource`
|
||||
* requires it and asserts on it. Redirects are followed by hand so a
|
||||
* HTTPS→HTTP downgrade can be refused and a permanent move can be reported to
|
||||
* the caller — see `dav/PROVENANCE.md` change 3.
|
||||
*/
|
||||
object CalDavHttp {
|
||||
|
||||
/**
|
||||
* One shared base client, so every derived client reuses its connection pool
|
||||
* and dispatcher threads. Building a fresh `OkHttpClient` per probe gives
|
||||
* each its own pool — every rung of the RFC 6764 ladder reopens TLS, and the
|
||||
* abandoned clients' idle threads live until GC.
|
||||
*/
|
||||
private val shared: OkHttpClient by lazy {
|
||||
OkHttpClient.Builder()
|
||||
.followRedirects(false)
|
||||
// A homelab server on the end of a slow link is normal; a hung socket
|
||||
// is not. Bounded so a killed worker is the exception, not the rule.
|
||||
.connectTimeout(30, TimeUnit.SECONDS)
|
||||
.readTimeout(120, TimeUnit.SECONDS)
|
||||
.writeTimeout(120, TimeUnit.SECONDS)
|
||||
.build()
|
||||
}
|
||||
|
||||
/** Discovery before we have credentials, and the Nextcloud login flow. */
|
||||
fun anonymous(userAgent: String): OkHttpClient = base(userAgent).build()
|
||||
|
||||
/**
|
||||
* Authenticated against [origin]'s registrable domain.
|
||||
*
|
||||
* Uses the vendored [BasicDigestAuthHandler] rather than a hand-rolled
|
||||
* interceptor, and it is worth saying why, because a preemptive-Basic
|
||||
* interceptor is the obvious thing to write and this project wrote one first:
|
||||
*
|
||||
* - **It does Digest.** Baïkal defaults to `dav_auth_type = Digest` and OkHttp
|
||||
* has no Digest support of its own (square/okhttp#205, open for years).
|
||||
* Baïkal is squarely in the self-hosting audience.
|
||||
* - **It already sends Basic preemptively over HTTPS**, and only over HTTPS,
|
||||
* so the extra round trip on every request of a PROPFIND-heavy sync is
|
||||
* avoided without a second implementation.
|
||||
* - **It restricts by registrable domain**, which is what a cross-host home
|
||||
* set needs: iCloud puts the principal on `caldav.icloud.com` and the home
|
||||
* set on `pNN-caldav.icloud.com`, and an exact-host allowlist refuses the
|
||||
* second one.
|
||||
* - It caches which scheme worked, so the challenge is paid once.
|
||||
*/
|
||||
fun authenticated(
|
||||
userAgent: String,
|
||||
username: String,
|
||||
password: String,
|
||||
origin: HttpUrl,
|
||||
): OkHttpClient {
|
||||
val handler = BasicDigestAuthHandler(
|
||||
// ⚠️ The **registrable** domain, not the host. The handler compares
|
||||
// its `domain` against UrlUtils.hostToDomain(request host), which
|
||||
// keeps only the last two labels — so passing "cloud.example.com"
|
||||
// compares it to "example.com", never matches, and the credential is
|
||||
// withheld from every request. That is every self-hosted Nextcloud.
|
||||
domain = UrlUtils.hostToDomain(origin.host),
|
||||
username = username,
|
||||
password = password,
|
||||
// Never preemptively over cleartext. The handler already gates its
|
||||
// own preemptive path on isHttps; stating it is cheap insurance.
|
||||
insecurePreemptive = false,
|
||||
)
|
||||
return base(userAgent)
|
||||
.authenticator(handler)
|
||||
.addNetworkInterceptor(handler)
|
||||
.build()
|
||||
}
|
||||
|
||||
private fun base(userAgent: String) = shared.newBuilder()
|
||||
.addInterceptor { chain ->
|
||||
chain.proceed(
|
||||
chain.request().newBuilder().header("User-Agent", userAgent).build(),
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -1,57 +0,0 @@
|
||||
package de.jeanlucmakiola.caldav
|
||||
|
||||
import okhttp3.Credentials
|
||||
import okhttp3.HttpUrl
|
||||
import okhttp3.Interceptor
|
||||
import okhttp3.Response
|
||||
|
||||
/**
|
||||
* Sends Basic credentials up front, rather than waiting to be challenged.
|
||||
*
|
||||
* OkHttp's `Authenticator` is **reactive only**: it fires after a 401, which
|
||||
* costs an extra round trip on every request of a PROPFIND-heavy sync — and it
|
||||
* never fires at all on servers that answer 403 or 404 without a challenge.
|
||||
*
|
||||
* Two guards, and neither is optional. The credential goes out **only over
|
||||
* HTTPS**, and **only to the account's own origin** — a home set may legally live
|
||||
* on another host, and OkHttp deliberately strips `Authorization` across a
|
||||
* redirect, so re-attaching it is something we must do knowingly, per host, after
|
||||
* validating the target. Never blanket.
|
||||
*/
|
||||
class PreemptiveBasicInterceptor(
|
||||
private val username: String,
|
||||
private val password: String,
|
||||
/** Hosts this credential may be sent to. */
|
||||
private val allowedHosts: Set<String>,
|
||||
) : Interceptor {
|
||||
|
||||
constructor(username: String, password: String, origin: HttpUrl) :
|
||||
this(username, password, setOf(origin.host))
|
||||
|
||||
/**
|
||||
* Lowercased once. OkHttp already lower-cases and punycodes `url.host`, so an
|
||||
* IDN written in Unicode here would never match — callers pass the host from
|
||||
* an [HttpUrl], which is already in that form.
|
||||
*/
|
||||
private val hosts = allowedHosts.map { it.lowercase() }.toSet()
|
||||
|
||||
override fun intercept(chain: Interceptor.Chain): Response {
|
||||
val request = chain.request()
|
||||
val url = request.url
|
||||
|
||||
val allowed = url.isHttps && url.host in hosts
|
||||
if (!allowed || request.header("Authorization") != null) {
|
||||
return chain.proceed(request)
|
||||
}
|
||||
|
||||
return chain.proceed(
|
||||
request.newBuilder()
|
||||
// ⚠️ UTF-8, not OkHttp's ISO-8859-1 default. A password with ä, ö
|
||||
// or ß is otherwise sent as different bytes than the server
|
||||
// expects — coming back as a 401 the user reads as "wrong
|
||||
// password". The vendored BasicDigestAuthHandler does the same.
|
||||
.header("Authorization", Credentials.basic(username, password, Charsets.UTF_8))
|
||||
.build(),
|
||||
)
|
||||
}
|
||||
}
|
||||
@@ -120,6 +120,19 @@ object ServiceDiscovery {
|
||||
if (into.none { it.url == url }) into += Candidate(url, label)
|
||||
}
|
||||
|
||||
/**
|
||||
* The server **root** for [input] — the origin, not the DAV path.
|
||||
*
|
||||
* Nextcloud's Login Flow v2 lives at `index.php/login/v2` off the root, so it
|
||||
* needs this rather than a discovered collection URL. A typed base URL is
|
||||
* taken as typed; an email address becomes `https://<domain>`.
|
||||
*/
|
||||
fun serverRootFor(input: String): HttpUrl? {
|
||||
val trimmed = input.trim()
|
||||
asBaseUrl(trimmed)?.let { return it }
|
||||
return domainOf(trimmed)?.let { "https://$it".toHttpUrlOrNull() }
|
||||
}
|
||||
|
||||
/** The input as a base URL, or null if it is an address rather than a URL. */
|
||||
internal fun asBaseUrl(input: String): HttpUrl? {
|
||||
if (!input.startsWith("http://", ignoreCase = true) &&
|
||||
|
||||
@@ -0,0 +1,102 @@
|
||||
package de.jeanlucmakiola.caldav
|
||||
|
||||
import at.bitfire.dav4jvm.BasicDigestAuthHandler
|
||||
import com.google.common.truth.Truth.assertThat
|
||||
import okhttp3.HttpUrl.Companion.toHttpUrl
|
||||
import okhttp3.Request
|
||||
import org.junit.Test
|
||||
|
||||
class CalDavHttpTest {
|
||||
|
||||
private val origin = "https://cloud.example.com/remote.php/dav/".toHttpUrl()
|
||||
|
||||
@Test
|
||||
fun `the auth handler is scoped to the registrable domain, not the host`() {
|
||||
// ⚠️ The handler compares its `domain` against
|
||||
// UrlUtils.hostToDomain(request host), which keeps only the last two
|
||||
// labels. Passing the full host means the comparison is
|
||||
// "cloud.example.com" == "example.com" — never true — and the credential
|
||||
// is withheld from every single request. That is every self-hosted
|
||||
// Nextcloud, silently answering 401 forever.
|
||||
val client = CalDavHttp.authenticated("Agendula", "user", "pw", origin)
|
||||
val handler = client.networkInterceptors.filterIsInstance<BasicDigestAuthHandler>().single()
|
||||
assertThat(handler.domain).isEqualTo("example.com")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `so the credential actually reaches the host it was made for`() {
|
||||
val client = CalDavHttp.authenticated("Agendula", "user", "pw", origin)
|
||||
val handler = client.networkInterceptors.filterIsInstance<BasicDigestAuthHandler>().single()
|
||||
|
||||
val authorised = handler.authenticateRequest(
|
||||
Request.Builder().url("https://cloud.example.com/remote.php/dav/").build(),
|
||||
null,
|
||||
)
|
||||
assertThat(authorised?.header("Authorization")).isNotNull()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `and reaches a sibling host in the same domain, which is what iCloud needs`() {
|
||||
// The principal is on caldav.icloud.com and the home set on
|
||||
// pNN-caldav.icloud.com. An exact-host allowlist refuses the second.
|
||||
val client = CalDavHttp.authenticated(
|
||||
"Agendula",
|
||||
"user",
|
||||
"pw",
|
||||
"https://caldav.icloud.com/".toHttpUrl(),
|
||||
)
|
||||
val handler = client.networkInterceptors.filterIsInstance<BasicDigestAuthHandler>().single()
|
||||
|
||||
val authorised = handler.authenticateRequest(
|
||||
Request.Builder().url("https://p42-caldav.icloud.com/1234/calendars/").build(),
|
||||
null,
|
||||
)
|
||||
assertThat(authorised?.header("Authorization")).isNotNull()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `an unrelated domain gets nothing`() {
|
||||
val client = CalDavHttp.authenticated("Agendula", "user", "pw", origin)
|
||||
val handler = client.networkInterceptors.filterIsInstance<BasicDigestAuthHandler>().single()
|
||||
|
||||
assertThat(
|
||||
handler.authenticateRequest(
|
||||
Request.Builder().url("https://evil.example.org/dav/").build(),
|
||||
null,
|
||||
),
|
||||
).isNull()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `cleartext never carries a preemptive credential`() {
|
||||
val client = CalDavHttp.authenticated("Agendula", "user", "pw", origin)
|
||||
val handler = client.networkInterceptors.filterIsInstance<BasicDigestAuthHandler>().single()
|
||||
|
||||
assertThat(
|
||||
handler.authenticateRequest(
|
||||
Request.Builder().url("http://cloud.example.com/dav/").build(),
|
||||
null,
|
||||
)?.header("Authorization"),
|
||||
).isNull()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `derived clients share one connection pool`() {
|
||||
// A fresh OkHttpClient per probe gives each its own pool and dispatcher
|
||||
// threads: every rung of the discovery ladder reopens TLS, and the
|
||||
// abandoned clients linger until GC.
|
||||
val a = CalDavHttp.anonymous("Agendula")
|
||||
val b = CalDavHttp.authenticated("Agendula", "user", "pw", origin)
|
||||
assertThat(a.connectionPool).isSameInstanceAs(b.connectionPool)
|
||||
assertThat(a.dispatcher).isSameInstanceAs(b.dispatcher)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `redirects are never followed automatically`() {
|
||||
// DavResource requires it and asserts on it: redirects are followed by
|
||||
// hand so a HTTPS-to-HTTP downgrade can be refused and a permanent move
|
||||
// reported.
|
||||
assertThat(CalDavHttp.anonymous("Agendula").followRedirects).isFalse()
|
||||
assertThat(CalDavHttp.authenticated("Agendula", "u", "p", origin).followRedirects).isFalse()
|
||||
}
|
||||
}
|
||||
@@ -1,102 +0,0 @@
|
||||
package de.jeanlucmakiola.caldav
|
||||
|
||||
import com.google.common.truth.Truth.assertThat
|
||||
import okhttp3.OkHttpClient
|
||||
import okhttp3.Request
|
||||
import okhttp3.mockwebserver.MockResponse
|
||||
import okhttp3.mockwebserver.MockWebServer
|
||||
import org.junit.After
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
|
||||
class PreemptiveBasicInterceptorTest {
|
||||
|
||||
private val server = MockWebServer()
|
||||
|
||||
@Before fun start() = server.start()
|
||||
@After fun stop() = server.shutdown()
|
||||
|
||||
private fun clientFor(allowedHosts: Set<String>) = OkHttpClient.Builder()
|
||||
.addInterceptor(PreemptiveBasicInterceptor("user", "pw", allowedHosts))
|
||||
.build()
|
||||
|
||||
private fun authHeaderOf(client: OkHttpClient): String? {
|
||||
server.enqueue(MockResponse().setResponseCode(200))
|
||||
client.newCall(Request.Builder().url(server.url("/dav/")).build()).execute().close()
|
||||
return server.takeRequest().getHeader("Authorization")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `plain HTTP never carries the credential, whatever the host list says`() {
|
||||
// MockWebServer is HTTP, so this also documents why the happy path below
|
||||
// is tested through the interceptor directly rather than over the wire.
|
||||
assertThat(authHeaderOf(clientFor(setOf(server.hostName)))).isNull()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a host outside the account's origin gets nothing`() {
|
||||
assertThat(authHeaderOf(clientFor(setOf("someone-else.example.com")))).isNull()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `the credential is attached up front on an allowed HTTPS host`() {
|
||||
// OkHttp's Authenticator is reactive-only: it costs an extra round trip on
|
||||
// every request of a PROPFIND-heavy sync, and never fires at all against a
|
||||
// server that answers 403 or 404 without a challenge.
|
||||
val interceptor = PreemptiveBasicInterceptor("user", "pw", setOf("cloud.example.com"))
|
||||
val request = Request.Builder().url("https://cloud.example.com/dav/").build()
|
||||
val chain = FakeChain(request)
|
||||
|
||||
interceptor.intercept(chain)
|
||||
|
||||
assertThat(chain.proceeded!!.header("Authorization")).isEqualTo("Basic dXNlcjpwdw==")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `an existing Authorization header is never overwritten`() {
|
||||
val interceptor = PreemptiveBasicInterceptor("user", "pw", setOf("cloud.example.com"))
|
||||
val request = Request.Builder()
|
||||
.url("https://cloud.example.com/dav/")
|
||||
.header("Authorization", "Digest something")
|
||||
.build()
|
||||
val chain = FakeChain(request)
|
||||
|
||||
interceptor.intercept(chain)
|
||||
|
||||
assertThat(chain.proceeded!!.header("Authorization")).isEqualTo("Digest something")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a cross-host home set does not silently receive the credential`() {
|
||||
// OkHttp strips Authorization across hosts on purpose. Re-attaching it is
|
||||
// something we do knowingly, per validated host — never blanket.
|
||||
val interceptor = PreemptiveBasicInterceptor("user", "pw", setOf("caldav.icloud.com"))
|
||||
val chain = FakeChain(Request.Builder().url("https://p42-caldav.icloud.com/dav/").build())
|
||||
|
||||
interceptor.intercept(chain)
|
||||
|
||||
assertThat(chain.proceeded!!.header("Authorization")).isNull()
|
||||
}
|
||||
|
||||
private class FakeChain(private val request: Request) : okhttp3.Interceptor.Chain {
|
||||
var proceeded: Request? = null
|
||||
override fun request() = request
|
||||
override fun proceed(request: Request): okhttp3.Response {
|
||||
proceeded = request
|
||||
return okhttp3.Response.Builder()
|
||||
.request(request)
|
||||
.protocol(okhttp3.Protocol.HTTP_1_1)
|
||||
.code(200)
|
||||
.message("OK")
|
||||
.build()
|
||||
}
|
||||
override fun connection() = null
|
||||
override fun call() = throw UnsupportedOperationException()
|
||||
override fun connectTimeoutMillis() = 0
|
||||
override fun withConnectTimeout(timeout: Int, unit: java.util.concurrent.TimeUnit) = this
|
||||
override fun readTimeoutMillis() = 0
|
||||
override fun withReadTimeout(timeout: Int, unit: java.util.concurrent.TimeUnit) = this
|
||||
override fun writeTimeoutMillis() = 0
|
||||
override fun withWriteTimeout(timeout: Int, unit: java.util.concurrent.TimeUnit) = this
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user