sync: what adding and re-adding an account actually has to do
Re-authentication reported success for work it never did. The branch took appPassword and dropped `found` and `selected` on the floor, so a user whose account had 401'd walked the whole add flow, ticked collections, and was shown Done — while no newly-ticked list was created, and a moved principal or corrected username was discarded, so a relocated account could never be repaired. It also never called accounts.add, so an account 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. Unticked lists are still left attached, deliberately: the picker pre-ticks what is *writable*, not what this account already holds, so detaching would stop syncing a read-only share over a default nobody chose. Remove-then-re-add duplicated every list. remove() leaves them behind as device-only lists on purpose and create() always inserted, so the user got their old Personal full of tasks beside a freshly synced Personal holding the same tasks from the server — the outcome the comment next to it names as the one to avoid. A collection whose href is already on the device is re-attached now, keeping its name, colour, ordering and tasks and losing only a cursor that belonged to the account that is gone. Rollback follows: it deletes what this attempt created and lets the FK return a re-attached list to being device-only. An account in the system with no Room row could not be removed from inside the app — accounts.remove's result is ignored and find returns null while the device is locked, so the pair can come apart, and every later add then answered AlreadyExists with the accounts screen driven off Room. That state is treated as an orphan to clean up. create()'s tail is uncancellable, like remove()'s: a back gesture during "Adding the account" left the row and its lists with no credential and no system account, where the rollback never runs, needsSignIn is false so re-auth will not fire, and every retry answers AlreadyExists. remove() forgets the quarantine counts as well as the cadence cursors — same keys, same globality, and a list re-attached to a new account would otherwise inherit a resource that is skipped for ever. And NeedsAuthentication's doc claimed the caller widens a credential allowlist with its hosts. Nothing does, and nothing should without the user's say-so; it says what the list is actually for.
This commit is contained in:
@@ -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<Long> {
|
||||
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<Long>()
|
||||
val accountId = database.runInTransaction<Long> {
|
||||
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<TaskCollection>): List<Long> {
|
||||
val orphans = database.taskLists().orphaned().associateBy { it.href }
|
||||
val inserted = mutableListOf<Long>()
|
||||
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<TaskCollection>,
|
||||
): 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<Long>) = 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
|
||||
|
||||
@@ -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<Long>) {
|
||||
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.
|
||||
|
||||
@@ -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<TaskListEntity>
|
||||
|
||||
/** 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<TaskListEntity>
|
||||
|
||||
@@ -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<String>) : Outcome
|
||||
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user