sync(chunk 5): attribution, revocation, compliance
The parts of chunk 5 that a build can verify. What is left needs a device or a live server, and is listed in docs/SYNC-PLAN.md rather than guessed at. - Attribution screen in Settings. dav4jvm is vendored, which makes MPL-2.0 §3.2(a) ours rather than a dependency's, so its row points at PROVENANCE.md next to upstream. Hand-maintained: generators read POM metadata, which routinely names a non-SPDX licence and a licence URL that 404s. - Revocation both ways. A 401 marks the account, stops it before the next request reaches the network, and takes it off the schedule from outside the worker — Nextcloud throttles then 429s per source IP, so a timer on a dead app password degrades the user's other clients. On removal, a bounded best-effort DELETE of the app password, or uninstalling never revokes it. - Play compliance: docs/PRIVACY.md linked in the app, declaring Collected and not Shared; an option to delete the account's tasks from the device too; REQUEST_IGNORE_BATTERY_OPTIMIZATIONS confirmed absent. - The server trap matrix as far as a protocol mock reaches, with four tests left @Ignore'd and their reasons written out. - docs/SYNC-PLAN.md records what moves to floret-kit, so that branch is a file move rather than a rediscovery. /code-review high raised 9 findings, all fixed. Three were serious: app-password revocation was aimed at the principal URL and revoked nothing; opening the app put accounts a 401 had stopped back on the timer, because KEEP does not keep cancelled work; and the incremental path advanced the sync token past bodies a failed multiget never applied. Also: four scalars were emitted twice whenever their residue copy survived, which the round-trip corpus could not see. Not done, and needing you: the live server matrix, releaseTest on device, the restore-onto-a-fresh-device check, cert4android, and MKCALENDAR feature detection. Chunk 2's on-device review is still outstanding.
This commit is contained in:
@@ -0,0 +1,82 @@
|
||||
package de.jeanlucmakiola.caldav
|
||||
|
||||
import okhttp3.HttpUrl
|
||||
import okhttp3.OkHttpClient
|
||||
import okhttp3.Request
|
||||
import java.io.IOException
|
||||
|
||||
/**
|
||||
* Gives an app password back when the account is removed.
|
||||
*
|
||||
* ⚠️ Without this, **uninstalling never revokes access**. Nextcloud's Login Flow
|
||||
* v2 mints a device-specific password that survives the app entirely: it stays
|
||||
* listed under Settings → Security → Devices & sessions until the user notices
|
||||
* and deletes it by hand, on an entry named after an app that is no longer
|
||||
* installed. Minting a credential and then abandoning it is not an acceptable
|
||||
* end state for a client that asked for one.
|
||||
*
|
||||
* Best-effort by design. The account is being removed either way, and a server
|
||||
* that is unreachable, or was never a Nextcloud, must not block that.
|
||||
*/
|
||||
object AppPassword {
|
||||
|
||||
/** Nextcloud's OCS endpoint for "delete the password I authenticated with". */
|
||||
private const val PATH = "ocs/v2.php/core/apppassword"
|
||||
|
||||
/**
|
||||
* Derives the OCS root from a Nextcloud principal URL.
|
||||
*
|
||||
* ⚠️ The principal URL is **not** the server root, and appending to it is the
|
||||
* bug this function exists to prevent: a principal is
|
||||
* `…/remote.php/dav/principals/users/alice/`, so
|
||||
* `principal + "ocs/v2.php/…"` produces a path that 404s on every server,
|
||||
* every time, silently — the revocation reads as "attempted" and does
|
||||
* nothing at all.
|
||||
*
|
||||
* Nextcloud mounts WebDAV under `remote.php`, so everything before that
|
||||
* segment is the server root, and that is true of a subpath install
|
||||
* (`https://host/nextcloud/`) as much as of a root one. Falling back to the
|
||||
* origin is right for a server that does not use `remote.php` — it is not a
|
||||
* Nextcloud, so the endpoint does not exist there under any path.
|
||||
*/
|
||||
fun ocsRootFor(principal: HttpUrl): HttpUrl {
|
||||
val mount = principal.pathSegments.indexOfFirst { it.equals("remote.php", ignoreCase = true) }
|
||||
// No `remote.php` means this is not a Nextcloud layout, and guessing a
|
||||
// prefix from an arbitrary DAV path would aim the DELETE somewhere
|
||||
// unrelated. The origin is the only defensible answer, and on a server
|
||||
// without the endpoint it simply 404s.
|
||||
val prefix = if (mount < 0) emptyList() else principal.pathSegments.take(mount)
|
||||
return principal.newBuilder()
|
||||
.encodedPath("/")
|
||||
.apply {
|
||||
prefix.filter { it.isNotEmpty() }.forEach { addPathSegment(it) }
|
||||
// A trailing empty segment keeps this a directory URL, so
|
||||
// appending the OCS path cannot fuse onto the last segment.
|
||||
if (prefix.any { it.isNotEmpty() }) addPathSegment("")
|
||||
}
|
||||
.build()
|
||||
}
|
||||
|
||||
/**
|
||||
* @return true when the server confirmed the revocation. False means the
|
||||
* credential may still exist server-side — the caller carries on regardless.
|
||||
*/
|
||||
fun revoke(httpClient: OkHttpClient, principal: HttpUrl): Boolean = try {
|
||||
val url = ocsRootFor(principal).newBuilder().addPathSegments(PATH).build()
|
||||
httpClient.newCall(
|
||||
Request.Builder()
|
||||
.url(url)
|
||||
.delete()
|
||||
// ⚠️ Not optional. Without this header Nextcloud answers the OCS
|
||||
// API with a 401 and a CSRF complaint rather than doing the work,
|
||||
// which reads exactly like a wrong password.
|
||||
.header("OCS-APIRequest", "true")
|
||||
.header("Accept", "application/json")
|
||||
.build(),
|
||||
).execute().use { it.isSuccessful }
|
||||
} catch (_: IOException) {
|
||||
false
|
||||
} catch (_: IllegalArgumentException) {
|
||||
false
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,62 @@
|
||||
package de.jeanlucmakiola.caldav
|
||||
|
||||
import com.google.common.truth.Truth.assertThat
|
||||
import okhttp3.HttpUrl.Companion.toHttpUrl
|
||||
import okhttp3.OkHttpClient
|
||||
import okhttp3.mockwebserver.MockResponse
|
||||
import okhttp3.mockwebserver.MockWebServer
|
||||
import org.junit.After
|
||||
import org.junit.Before
|
||||
import org.junit.Test
|
||||
|
||||
class AppPasswordTest {
|
||||
|
||||
private val server = MockWebServer()
|
||||
private val httpClient = OkHttpClient()
|
||||
|
||||
@Before fun start() = server.start()
|
||||
|
||||
@After fun stop() = server.shutdown()
|
||||
|
||||
@Test fun `the OCS root is derived from a principal, not appended to it`() {
|
||||
// ⚠️ Appending to the principal produces a path that 404s on every
|
||||
// server, silently — the revocation looks attempted and does nothing.
|
||||
val principal = "https://cloud.example.com/remote.php/dav/principals/users/alice/"
|
||||
assertThat(AppPassword.ocsRootFor(principal.toHttpUrl()).toString())
|
||||
.isEqualTo("https://cloud.example.com/")
|
||||
}
|
||||
|
||||
@Test fun `a subpath install keeps its subpath`() {
|
||||
val principal = "https://host/nextcloud/remote.php/dav/principals/users/alice/"
|
||||
assertThat(AppPassword.ocsRootFor(principal.toHttpUrl()).toString())
|
||||
.isEqualTo("https://host/nextcloud/")
|
||||
}
|
||||
|
||||
@Test fun `a principal with no remote_php falls back to the origin`() {
|
||||
val principal = "https://baikal.example.com/dav.php/principals/alice/"
|
||||
assertThat(AppPassword.ocsRootFor(principal.toHttpUrl()).toString())
|
||||
.isEqualTo("https://baikal.example.com/")
|
||||
}
|
||||
|
||||
@Test fun `the revocation is a DELETE with the OCS header`() {
|
||||
server.enqueue(MockResponse().setResponseCode(200))
|
||||
|
||||
val revoked = AppPassword.revoke(
|
||||
httpClient,
|
||||
server.url("/remote.php/dav/principals/users/alice/"),
|
||||
)
|
||||
|
||||
assertThat(revoked).isTrue()
|
||||
val request = server.takeRequest()
|
||||
assertThat(request.method).isEqualTo("DELETE")
|
||||
assertThat(request.path).isEqualTo("/ocs/v2.php/core/apppassword")
|
||||
// ⚠️ Without this header Nextcloud answers with a CSRF complaint that
|
||||
// reads exactly like a wrong password.
|
||||
assertThat(request.getHeader("OCS-APIRequest")).isEqualTo("true")
|
||||
}
|
||||
|
||||
@Test fun `an unreachable server is reported, not thrown`() {
|
||||
server.enqueue(MockResponse().setResponseCode(404))
|
||||
assertThat(AppPassword.revoke(httpClient, server.url("/remote.php/dav/"))).isFalse()
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,180 @@
|
||||
package de.jeanlucmakiola.caldav
|
||||
|
||||
import com.google.common.truth.Truth.assertThat
|
||||
import okhttp3.OkHttpClient
|
||||
import okhttp3.mockwebserver.MockResponse
|
||||
import okhttp3.mockwebserver.MockWebServer
|
||||
import org.junit.After
|
||||
import org.junit.Before
|
||||
import org.junit.Ignore
|
||||
import org.junit.Test
|
||||
|
||||
/**
|
||||
* The per-server trap matrix from `docs/SYNC.md` § *Server reality*.
|
||||
*
|
||||
* Everything reproducible from the protocol alone runs here against
|
||||
* MockWebServer. The handful that genuinely need a live server are `@Ignore`d
|
||||
* with the reason spelled out — they are named so they can be turned on by hand,
|
||||
* not deleted and rediscovered.
|
||||
*/
|
||||
class ServerMatrixTest {
|
||||
|
||||
private val server = MockWebServer()
|
||||
private val httpClient = OkHttpClient.Builder().followRedirects(false).build()
|
||||
private lateinit var collection: CalendarCollection
|
||||
|
||||
@Before fun start() {
|
||||
server.start()
|
||||
collection = CalendarCollection(httpClient, server.url("/dav/tasks/"))
|
||||
}
|
||||
|
||||
@After fun stop() = server.shutdown()
|
||||
|
||||
// ------------------------------------------------------------- Nextcloud
|
||||
|
||||
@Test fun `Nextcloud per-calendar UID uniqueness is a rejection, not a retry`() {
|
||||
// 409 no-uid-conflict: another resource in this collection already has
|
||||
// that UID. Retrying the same body reproduces it exactly.
|
||||
server.enqueue(
|
||||
MockResponse().setResponseCode(409)
|
||||
.setHeader("Content-Type", "application/xml; charset=utf-8")
|
||||
.setBody(
|
||||
"<D:error xmlns:D=\"DAV:\" xmlns:C=\"urn:ietf:params:xml:ns:caldav\">" +
|
||||
"<C:no-uid-conflict/></D:error>",
|
||||
),
|
||||
)
|
||||
|
||||
val outcome = collection.create("one.ics", "x")
|
||||
|
||||
assertThat(outcome).isInstanceOf(PutOutcome.Rejected::class.java)
|
||||
assertThat((outcome as PutOutcome.Rejected).code).isEqualTo(409)
|
||||
}
|
||||
|
||||
@Test fun `Nextcloud's trashbin makes a reused href answer 403`() {
|
||||
// The trashbin renames a deleted resource to `<name>-deleted.ics`, so
|
||||
// delete, recreate and delete again at the same href returns 403. Task
|
||||
// apps hit this constantly because they reuse hrefs.
|
||||
server.enqueue(MockResponse().setResponseCode(403))
|
||||
|
||||
val outcome = collection.delete(server.url("/dav/tasks/one.ics"), "e")
|
||||
|
||||
// Not "already gone" and not a conflict — a refusal that must be
|
||||
// quarantined rather than retried forever.
|
||||
assertThat(outcome).isInstanceOf(DeleteOutcome.Rejected::class.java)
|
||||
assertThat((outcome as DeleteOutcome.Rejected).code).isEqualTo(403)
|
||||
}
|
||||
|
||||
@Test fun `a shared Nextcloud calendar is recognised as shared`() {
|
||||
// ⚠️ The whole mutilation guard hangs off this bit. `CalendarObject::get()`
|
||||
// serves a whitelist-reduced body from a share while leaving the ETag
|
||||
// untouched, so an engine that thinks the collection is not shared will
|
||||
// write that reduced body back over the owner's task.
|
||||
server.enqueue(
|
||||
MockResponse().setResponseCode(207)
|
||||
.setHeader("Content-Type", "application/xml; charset=utf-8")
|
||||
.setBody(
|
||||
"""
|
||||
<multistatus xmlns="DAV:">
|
||||
<response>
|
||||
<href>/dav/tasks/</href>
|
||||
<propstat><prop><resourcetype>
|
||||
<collection/>
|
||||
<C:calendar xmlns:C="urn:ietf:params:xml:ns:caldav"/>
|
||||
<CS:shared xmlns:CS="http://calendarserver.org/ns/"/>
|
||||
</resourcetype></prop><status>HTTP/1.1 200 OK</status></propstat>
|
||||
</response>
|
||||
</multistatus>
|
||||
""".trimIndent(),
|
||||
),
|
||||
)
|
||||
|
||||
assertThat(collection.state().getOrThrow().collection.isShared).isTrue()
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------ SOGo
|
||||
|
||||
@Test fun `SOGo's second-granularity token is carried verbatim`() {
|
||||
// SOGo's tokens are second-granularity integers rather than URIs. Nothing
|
||||
// may parse, normalise or compare them as anything but opaque text.
|
||||
server.enqueue(
|
||||
MockResponse().setResponseCode(207)
|
||||
.setHeader("Content-Type", "application/xml; charset=utf-8")
|
||||
.setBody(
|
||||
"<multistatus xmlns=\"DAV:\"><sync-token>1730000000</sync-token></multistatus>",
|
||||
),
|
||||
)
|
||||
|
||||
val page = collection.changes("1729999999") as ChangeSet.Page
|
||||
|
||||
assertThat(page.token).isEqualTo("1730000000")
|
||||
assertThat(server.takeRequest().body.readUtf8()).contains("1729999999")
|
||||
}
|
||||
|
||||
@Test fun `SOGo's main calendar survives classification`() {
|
||||
// ⚠️ SOGo reports collection + calendar + schedule-outbox together for
|
||||
// every non-Apple client. The obvious "exclude schedule-outbox" rule
|
||||
// drops the user's only calendar.
|
||||
server.enqueue(
|
||||
MockResponse().setResponseCode(207)
|
||||
.setHeader("Content-Type", "application/xml; charset=utf-8")
|
||||
.setBody(
|
||||
"""
|
||||
<multistatus xmlns="DAV:">
|
||||
<response>
|
||||
<href>/dav/tasks/</href>
|
||||
<propstat><prop><resourcetype>
|
||||
<collection/>
|
||||
<C:calendar xmlns:C="urn:ietf:params:xml:ns:caldav"/>
|
||||
<C:schedule-outbox xmlns:C="urn:ietf:params:xml:ns:caldav"/>
|
||||
</resourcetype></prop><status>HTTP/1.1 200 OK</status></propstat>
|
||||
</response>
|
||||
</multistatus>
|
||||
""".trimIndent(),
|
||||
),
|
||||
)
|
||||
|
||||
assertThat(collection.state().isSuccess).isTrue()
|
||||
}
|
||||
|
||||
// -------------------------------------------------------------- Radicale
|
||||
|
||||
@Test fun `Radicale advertising sync-collection without meaning it degrades`() {
|
||||
// Radicale advertised the report for years without implementing it, so a
|
||||
// 501 must fall back rather than fail the collection.
|
||||
server.enqueue(MockResponse().setResponseCode(501))
|
||||
assertThat(collection.changes("t")).isEqualTo(ChangeSet.Unsupported)
|
||||
}
|
||||
|
||||
// ---------------------------------------------------- needs a real server
|
||||
|
||||
@Ignore(
|
||||
"Needs a live Baikal on dav_auth_type = Digest. OkHttp has no Digest of " +
|
||||
"its own (square/okhttp#205), so this exercises the vendored " +
|
||||
"BasicDigestAuthHandler against a real challenge/nonce cycle, which " +
|
||||
"MockWebServer cannot reproduce faithfully.",
|
||||
)
|
||||
@Test fun `Baikal on Digest authenticates`() = Unit
|
||||
|
||||
@Ignore(
|
||||
"Needs a live Nextcloud with a calendar shared read-write from another " +
|
||||
"account, holding a CLASS:CONFIDENTIAL task. Verifies that we never " +
|
||||
"write back the whitelist-reduced body CalendarObject::get() serves " +
|
||||
"— the single most destructive bug available here, and one no mock " +
|
||||
"can prove absent because the reduction happens server-side.",
|
||||
)
|
||||
@Test fun `a confidential task on a shared Nextcloud calendar survives a round trip`() = Unit
|
||||
|
||||
@Ignore(
|
||||
"Needs a live SOGo. Its ETag is a row-version counter and the body is " +
|
||||
"regenerated per principal, so the same ETag can accompany different " +
|
||||
"bytes. Proving we do not silently keep a stale body needs the real " +
|
||||
"server's regeneration behaviour.",
|
||||
)
|
||||
@Test fun `an ETag-unchanged body change on SOGo is detected`() = Unit
|
||||
|
||||
@Ignore(
|
||||
"Needs a live Nextcloud. MKCALENDAR is rate-limited to 10 per hour, " +
|
||||
"which is the behaviour under test and cannot be mocked usefully.",
|
||||
)
|
||||
@Test fun `Nextcloud rate-limits collection creation`() = Unit
|
||||
}
|
||||
Reference in New Issue
Block a user