diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt index 2c33b49..0dfaf99 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt @@ -50,6 +50,7 @@ class AccountRepository @Inject constructor( private val syncTrigger: SyncTrigger, private val cadence: SyncCadenceStore, private val accountState: AccountStateStore, + private val quarantine: QuarantineStore, private val gateway: CalDavGateway, @IoDispatcher private val io: CoroutineDispatcher, ) : AccountCreator { @@ -99,7 +100,7 @@ class AccountRepository @Inject constructor( // accounts.display_name and nothing prunes Room when the system account // disappears, so "removed from system Settings, re-added here" would // otherwise leave a second Room row and a duplicate of every list. - val existsInSystem = accounts.find(displayName) != null + val systemAccount = accounts.find(displayName) val existing = database.accounts().all().firstOrNull { it.displayName == displayName } // ⚠️ Re-authentication, not a duplicate. An account stopped by a 401 has @@ -110,102 +111,199 @@ class AccountRepository @Inject constructor( // same name is still a duplicate, so this can never silently overwrite a // working credential. if (existing != null && accountState.needsSignIn(existing.id)) { - return@withContext reauthenticate(existing.id, displayName, appPassword) + return@withContext reauthenticate(existing, displayName, username, appPassword, found, selected) } - if (existsInSystem || existing != null) return@withContext Outcome.AlreadyExists + if (existing != null) return@withContext Outcome.AlreadyExists - val accountId = database.runInTransaction { - val id = database.accounts().insert( - AccountEntity( - displayName = displayName, - // Persist where a 301/308 actually put us — dav4jvm#209 exists - // precisely so this is knowable, and re-following the redirect - // on every sync is what not persisting it costs. - principalUrl = (found.movedTo ?: found.principal).toString(), - // The principal's own home set. `resolve("./")` on a collection - // URL is a no-op — CalDAV hrefs already end in "/" — so the - // old version stored the first collection's own URL, and that - // collection may not even be from the account's own home set. - homeSetUrl = found.homeSets.firstOrNull()?.toString(), - username = username, - ), - ) - selected.forEach { collection -> - database.taskLists().insert( + // ⚠️ In the system, not in Room: an orphan, not a duplicate. `remove()` + // ignores whether the AccountManager entry actually went (and `find` + // returns null while the device is locked), so this state is reachable — + // and refusing here left the user with an account that cannot be removed + // from inside the app at all, since the accounts screen is driven off + // Room. Clearing it up is kinder than refusing for ever. + systemAccount?.let { accounts.remove(it) } + + // ⚠️ Uncancellable as a whole. The row, its lists, the credential and + // the system entry are four stores that cannot share a transaction, and + // the caller is a viewModelScope tied to the Settings destination — a + // back gesture during "Adding the account" would otherwise leave the row + // and its lists with no credential and no system account: the rollback + // never runs, `needsSignIn` is false so re-auth will not fire, and every + // retry answers AlreadyExists. `remove()` documents the same hazard. + withContext(NonCancellable) { + val inserted = mutableListOf() + val accountId = database.runInTransaction { + val id = database.accounts().insert( + AccountEntity( + displayName = displayName, + // Persist where a 301/308 actually put us — dav4jvm#209 exists + // precisely so this is knowable, and re-following the redirect + // on every sync is what not persisting it costs. + principalUrl = (found.movedTo ?: found.principal).toString(), + // The principal's own home set. `resolve("./")` on a collection + // URL is a no-op — CalDAV hrefs already end in "/" — so the + // old version stored the first collection's own URL, and that + // collection may not even be from the account's own home set. + homeSetUrl = found.homeSets.firstOrNull()?.toString(), + username = username, + ), + ) + inserted += attach(id, selected) + id + } + + if (!credentials.put(accountId, appPassword)) { + // Never leave a half-made account behind: without a credential it + // would sit in Settings failing to sync with nothing to explain it. + rollback(accountId, inserted) + return@withContext Outcome.CredentialFailed( + Outcome.Cause.KEYSTORE_REFUSED, + "the device keystore would not store the password", + ) + } + + if (!accounts.add(displayName, accountId)) { + credentials.clear(accountId) + rollback(accountId, inserted) + return@withContext Outcome.AlreadyExists + } + + // On the schedule from the moment it exists, and syncing immediately — + // an account that shows up empty until the first periodic window looks + // broken. + syncTrigger.schedule(displayName) + syncTrigger.enqueue(displayName) + + Outcome.Created(accountId) + } + } + + /** + * Points [selected] at [accountId], re-attaching what a previous removal + * left behind rather than inserting a second copy. + * + * ⚠️ `remove()` leaves the lists as device-only ones on purpose, so a plain + * insert gives the user their old "Personal" full of tasks *and* a freshly + * synced "Personal" holding the same tasks from the server. The row keeps + * its name, colour, ordering and tasks — the user's, not the server's — and + * loses only its cursor, because the account it was reconciled against is + * gone. + * + * @return the ids of the lists this call *created*, which are the only ones + * a rollback may delete. + */ + private fun attach(accountId: Long, selected: Set): List { + val orphans = database.taskLists().orphaned().associateBy { it.href } + val inserted = mutableListOf() + selected.forEach { collection -> + val href = collection.url.toString() + val orphan = orphans[href] + if (orphan == null) { + inserted += database.taskLists().insert( TaskListEntity( name = collection.displayName ?: collection.url.pathSegments .lastOrNull { it.isNotEmpty() } .orEmpty(), color = collection.color ?: DEFAULT_LIST_COLOR, - accountId = id, + accountId = accountId, isReadOnly = collection.readOnly, - href = collection.url.toString(), + href = href, + ), + ) + } else { + database.taskLists().update( + orphan.copy( + accountId = accountId, + isReadOnly = collection.readOnly, + // A cursor from the account that used to own this list + // says nothing about the one that owns it now. + syncToken = null, + ctag = null, ), ) } - id } - - if (!credentials.put(accountId, appPassword)) { - // Never leave a half-made account behind: without a credential it - // would sit in Settings failing to sync with nothing to explain it. - rollback(accountId) - return@withContext Outcome.CredentialFailed( - Outcome.Cause.KEYSTORE_REFUSED, - "the device keystore would not store the password", - ) - } - - if (!accounts.add(displayName, accountId)) { - credentials.clear(accountId) - rollback(accountId) - return@withContext Outcome.AlreadyExists - } - - // On the schedule from the moment it exists, and syncing immediately — - // an account that shows up empty until the first periodic window looks - // broken. - syncTrigger.schedule(displayName) - syncTrigger.enqueue(displayName) - - Outcome.Created(accountId) + return inserted } /** * Replaces the credential of an account the server had stopped accepting. * - * The lists, their sync tokens and every task stay exactly as they are — - * the password was the only thing that went stale. + * The tasks stay exactly as they are — the password was the only thing that + * went stale. Everything the user was asked for on the way here is applied + * all the same: + * + * ⚠️ This branch used to take [appPassword] and drop [found] and [selected] + * on the floor. The user walked the whole add flow, ticked collections, and + * was shown Done — while no newly-ticked list was created, no unticked one + * was detached, and a moved principal or a corrected username was discarded, + * so a relocated account could never be repaired. It also never called + * `accounts.add`, so an account the user had deleted in Android Settings — + * nothing listens for `LOGIN_ACCOUNTS_CHANGED` — stayed absent from Settings + * for ever while syncing happily via WorkManager, and every later add + * answered AlreadyExists. */ private suspend fun reauthenticate( - accountId: Long, + existing: AccountEntity, displayName: String, + username: String, appPassword: String, + found: CalDavDiscovery.Outcome.Found, + selected: Set, ): Outcome { + val accountId = existing.id if (!credentials.put(accountId, appPassword)) { return Outcome.CredentialFailed( Outcome.Cause.KEYSTORE_REFUSED, "the device keystore would not store the password", ) } - accountState.setNeedsSignIn(accountId, false) - database.accounts().recordSync(accountId, at = null, error = null) - syncTrigger.schedule(displayName) - syncTrigger.enqueue(displayName) + withContext(NonCancellable) { + database.runInTransaction { + database.accounts().update( + existing.copy( + // Where discovery just found it, which is the only way a + // principal that has moved can ever be corrected. + principalUrl = (found.movedTo ?: found.principal).toString(), + homeSetUrl = found.homeSets.firstOrNull()?.toString() + ?: existing.homeSetUrl, + username = username, + ), + ) + attach(accountId, selected) + } + // ⚠️ Ticked lists are attached; unticked ones are left alone. The + // picker pre-ticks everything *writable*, not everything already + // attached, so detaching what is unticked would silently stop + // syncing a read-only share the account has synced for months — + // over a default the user never chose. Detaching belongs here the + // day the picker knows what this account already holds. + // The Room row is the account as far as this app is concerned, so a + // missing system entry is re-registered rather than left behind. + if (accounts.find(displayName) == null) accounts.add(displayName, accountId) + accountState.setNeedsSignIn(accountId, false) + database.accounts().recordSync(accountId, at = null, error = null) + syncTrigger.schedule(displayName) + syncTrigger.enqueue(displayName) + } return Outcome.Created(accountId) } /** * Undoes a half-made account. * - * ⚠️ The lists have to go **explicitly**. `task_lists.account_id` is - * `ON DELETE SET NULL` — deliberately, so removing a working account never - * destroys tasks — which means deleting the account row alone leaves a full - * set of orphan device-only lists behind, and every retry adds another. + * ⚠️ The lists this attempt *created* have to go explicitly. + * `task_lists.account_id` is `ON DELETE SET NULL` — deliberately, so + * removing a working account never destroys tasks — which means deleting the + * account row alone would leave a set of empty device-only lists behind. + * + * ⚠️ And only those. A list [attach] re-attached was already on the device + * and holds the user's tasks; `SET NULL` returns it to being device-only, + * which is exactly where it came from. */ - private fun rollback(accountId: Long) = database.runInTransaction { - database.taskLists().deleteForAccount(accountId) + private fun rollback(accountId: Long, inserted: List) = database.runInTransaction { + inserted.forEach { database.taskLists().delete(it) } database.accounts().delete(accountId) } @@ -271,9 +369,15 @@ class AccountRepository @Inject constructor( // The lists survive as device-only lists, so their cursors must // not: a re-added account would otherwise inherit a "reconciled // recently" that was true of a different account's data. - cadence.forget( - database.taskLists().syncedForAccount(accountId).map { it.id }.toSet(), - ) + val listIds = database.taskLists().syncedForAccount(accountId) + .map { it.id } + .toSet() + cadence.forget(listIds) + // ⚠️ And the quarantine counts, which are keyed the same way and + // are just as global. A list re-attached to a new account would + // otherwise inherit them, and a resource already at THRESHOLD is + // skipped for ever — it never succeeds, so it never clears. + quarantine.forget(listIds) // Play's Account Deletion policy does not apply to us — there is // no Agendula account to delete — but "I want it gone from this // device too" is a reasonable thing to want, and it is the only diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/QuarantineStore.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/QuarantineStore.kt index e8b48d9..7d9f0b3 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/QuarantineStore.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/QuarantineStore.kt @@ -61,6 +61,27 @@ class QuarantineStore @Inject constructor( } } + /** + * Forgets every count belonging to [listIds]. + * + * ⚠️ The keys are global, exactly like the cadence cursors cleared beside + * them. A list detached from a removed account and re-attached to a new one + * would otherwise inherit its old counters — and a resource already at + * [THRESHOLD] is skipped for ever, since a quarantined resource never + * succeeds and so never clears. + */ + suspend fun forget(listIds: Set) { + if (listIds.isEmpty()) return + val prefixes = listIds.map { "$it|" } + dataStore.edit { prefs -> + val current = decode(prefs[KEY].orEmpty()) + .filterKeys { key -> prefixes.none(key::startsWith) } + prefs[KEY] = current + .map { (key, count) -> "$key$COUNT_SEPARATOR$count" } + .toSet() + } + } + companion object { /** * Attempts before a resource is left alone. diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt index f0e4858..4e719b1 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt @@ -83,6 +83,18 @@ interface TaskListDao { @Query("UPDATE task_lists SET sync_token = :token WHERE id = :listId") fun setSyncToken(listId: Long, token: String?) + /** + * Lists an account left behind: detached, but still naming a collection. + * + * ⚠️ `remove()` deliberately leaves the lists as device-only ones, so + * re-adding the same account must *re-attach* these rather than insert a + * second copy of every collection — the user would otherwise get their old + * "Personal" full of tasks beside a freshly synced "Personal" holding the + * same tasks from the server. + */ + @Query("SELECT * FROM task_lists WHERE account_id IS NULL AND href IS NOT NULL") + fun orphaned(): List + /** The synced collections of one account, in the order sync walks them. */ @Query("SELECT * FROM task_lists WHERE account_id = :accountId AND is_synced = 1 ORDER BY id") fun syncedForAccount(accountId: Long): List diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt index 8e310ca..3bb6a26 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt @@ -65,12 +65,18 @@ class CalDavDiscovery( /** * The server wants credentials. Not a failure — authenticate and retry. * - * [hosts] names *which* hosts asked, which the caller needs in order to - * widen the credential allowlist. A cross-host home set (iCloud puts the - * principal on `caldav.icloud.com` and the home set on - * `pNN-caldav.icloud.com`) is otherwise unreachable: the interceptor - * withholds the credential from the second host, and without naming it - * here the caller can never learn what to allow. + * [hosts] names *which* hosts asked, so the caller can say which one it + * could not reach. A cross-host home set under the same registrable + * domain — iCloud puts the principal on `caldav.icloud.com` and the home + * set on `pNN-caldav.icloud.com` — is already covered, because that is + * the scope `CalDavHttp` gives the credential. + * + * ⚠️ What is *not* covered is a home set on a genuinely different + * registrable domain, which RFC 4791 §6.2.1 allows. Nothing widens the + * allowlist for it and nothing should without the user's say-so: the + * password would be offered to a host named by the first server, and one + * credential scope is what makes that decidable. The add flow reports + * such a host by name instead of pretending the account is empty. */ data class NeedsAuthentication(val hosts: List) : Outcome diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt index f518159..a6988ab 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt @@ -256,7 +256,14 @@ class NextcloudLoginFlow( */ internal fun secureOrigin(expected: HttpUrl, actual: HttpUrl): HttpUrl = if (expected.isHttps && !actual.isHttps) { - actual.newBuilder().scheme("https").build() + // ⚠️ The port goes with the scheme. OkHttp only drops a *default* + // port across a scheme change, so `http://cloud.example.com:8080/` + // coerced to https keeps :8080 — a port that almost certainly speaks + // cleartext, and the promised coercion becomes a TLS handshake + // failure. The port that answered our poll is the one known to work, + // and a claimed port emitted alongside a wrong scheme comes from the + // same misconfiguration as the scheme did. + actual.newBuilder().scheme("https").port(expected.port).build() } else { actual } diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt index ccc9d8e..f4a99d1 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt @@ -198,6 +198,21 @@ class NextcloudLoginFlowTest { assertThat(coerced.host).isEqualTo("cloud.example.com") } + @Test fun `a cleartext port does not survive the coercion`() { + val flow = NextcloudLoginFlow(OkHttpClient(), "test") + val expected = "https://cloud.example.com/login/v2/poll".toHttpUrl() + val downgraded = "http://cloud.example.com:8080/".toHttpUrl() + + // ⚠️ OkHttp only drops a *default* port across a scheme change, so + // :8080 would be carried into https — a port that almost certainly + // speaks cleartext, turning the promised coercion into a handshake + // failure. The port that answered the poll is the one known to work. + val coerced = flow.secureOrigin(expected, downgraded) + + assertThat(coerced.scheme).isEqualTo("https") + assertThat(coerced.port).isEqualTo(443) + } + @Test fun `an already-secure server URL is left alone`() { val flow = NextcloudLoginFlow(OkHttpClient(), "test") val expected = "https://cloud.example.com/login/v2/poll".toHttpUrl()