diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/MainActivity.kt b/app/src/main/java/de/jeanlucmakiola/agendula/MainActivity.kt index 8406e5f..709d3c4 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/MainActivity.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/MainActivity.kt @@ -140,5 +140,18 @@ class MainActivity : ComponentActivity() { putExtra(EXTRA_TASK_ID, taskId) addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) } + + /** + * Just opens the app. + * + * What the sync notice taps into. Settings → Accounts is where it + * would rather go, and it will once routing lands — the same work + * [taskIntent] is waiting on — but a notification that opens the app + * is still the difference between the user hearing about a discarded + * edit and not. + */ + fun openIntent(context: Context): Intent = + Intent(context, MainActivity::class.java) + .addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) } } 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 d5a2f1a..927dad8 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 @@ -51,6 +51,8 @@ class AccountRepository @Inject constructor( private val cadence: SyncCadenceStore, private val accountState: AccountStateStore, private val quarantine: QuarantineStore, + private val notices: SyncNoticeStore, + private val collectionSupport: CollectionSupportStore, private val gateway: CalDavGateway, @IoDispatcher private val io: CoroutineDispatcher, ) : AccountCreator { @@ -394,6 +396,13 @@ class AccountRepository @Inject constructor( if (deleteLocalData) database.taskLists().deleteForAccount(accountId) accountState.setNeedsSignIn(accountId, false) + // Keyed by account id, exactly like the flag above, and just as + // orphaned once the row goes: ids are AUTOINCREMENT so they are + // never reused, but nothing would ever read or clear these again. + notices.dismiss(accountId) + // The same reasoning, for what the server said it would let us + // create: keyed by an id nothing will ever mention again. + collectionSupport.forget(accountId) credentials.clear(accountId) database.accounts().delete(accountId) accounts.find(displayName)?.let { accounts.remove(it) } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSupportStore.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSupportStore.kt new file mode 100644 index 0000000..85503e9 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSupportStore.kt @@ -0,0 +1,72 @@ +package de.jeanlucmakiola.agendula.data.sync + +import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.edit +import androidx.datastore.preferences.core.stringSetPreferencesKey +import de.jeanlucmakiola.agendula.data.di.SyncStateDataStore +import de.jeanlucmakiola.caldav.CollectionSupport +import kotlinx.coroutines.flow.first +import javax.inject.Inject +import javax.inject.Singleton + +/** + * What each account's home set answered to OPTIONS, last time we asked. + * + * ⚠️ A cache, never the answer. `docs/SYNC-PLAN.md` chunk 5 asks for the "new + * task list" affordance to be *hidden* where neither MKCALENDAR nor extended + * MKCOL exists, and a picker that has to make a network round trip before it can + * draw a row is a picker that stutters — so the last answer is what it draws + * with, and [RemoteListRepository] re-asks before it actually writes. A server + * that gained the capability in an upgrade, or lost it in a config change, is + * then wrong for exactly one glance rather than for ever. + */ +@Singleton +class CollectionSupportStore @Inject constructor( + @SyncStateDataStore private val dataStore: DataStore, +) { + + suspend fun get(accountId: Long): CollectionSupport = + decode(dataStore.data.first()[KEY].orEmpty())[accountId] ?: CollectionSupport.NONE + + /** Asks [ask], records what it said, and hands it back. */ + suspend fun refresh(accountId: Long, ask: () -> CollectionSupport): CollectionSupport { + val answer = ask() + dataStore.edit { prefs -> + // Re-read inside `edit`, which DataStore serialises: two accounts + // can be asked at once and a snapshot taken outside would drop one. + val current = decode(prefs[KEY].orEmpty()).toMutableMap() + current[accountId] = answer + prefs[KEY] = current.map { (id, support) -> encode(id, support) }.toSet() + } + return answer + } + + suspend fun forget(accountId: Long) { + dataStore.edit { prefs -> + prefs[KEY] = decode(prefs[KEY].orEmpty()) + .filterKeys { it != accountId } + .map { (id, support) -> encode(id, support) } + .toSet() + } + } + + private fun encode(accountId: Long, support: CollectionSupport): String = + "$accountId|${support.mkCalendar}|${support.extendedMkCol}" + + private fun decode(entries: Set): Map = + entries.mapNotNull { entry -> + val parts = entry.split('|') + if (parts.size != FIELDS) return@mapNotNull null + val accountId = parts[0].toLongOrNull() ?: return@mapNotNull null + accountId to CollectionSupport( + mkCalendar = parts[1].toBooleanStrictOrNull() ?: return@mapNotNull null, + extendedMkCol = parts[2].toBooleanStrictOrNull() ?: return@mapNotNull null, + ) + }.toMap() + + private companion object { + val KEY = stringSetPreferencesKey("collection_support") + const val FIELDS = 3 + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/RemoteListRepository.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/RemoteListRepository.kt new file mode 100644 index 0000000..7d87e17 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/RemoteListRepository.kt @@ -0,0 +1,308 @@ +package de.jeanlucmakiola.agendula.data.sync + +import de.jeanlucmakiola.agendula.data.di.IoDispatcher +import de.jeanlucmakiola.agendula.data.tasks.ProviderResolver +import de.jeanlucmakiola.agendula.data.tasks.StorageMode +import de.jeanlucmakiola.agendula.data.tasks.room.AccountEntity +import de.jeanlucmakiola.agendula.data.tasks.room.TaskListEntity +import de.jeanlucmakiola.agendula.data.tasks.room.TasksDatabase +import de.jeanlucmakiola.caldav.CalDavHttp +import de.jeanlucmakiola.caldav.CollectionAdmin +import de.jeanlucmakiola.caldav.CollectionOutcome +import de.jeanlucmakiola.caldav.CollectionSupport +import de.jeanlucmakiola.caldav.DavCollectionAdmin +import de.jeanlucmakiola.caldav.ResourceNames +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.NonCancellable +import kotlinx.coroutines.async +import kotlinx.coroutines.awaitAll +import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.withContext +import okhttp3.HttpUrl +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull +import okhttp3.OkHttpClient +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Task lists that live on a server: making them, renaming them, recolouring + * them and deleting them. + * + * ⚠️ Every write here is **server first, Room second**, and that ordering is the + * whole design. The other way round gives the user a list that exists on their + * phone and nowhere else, and nothing to tell them so — `task_lists.is_dirty` + * was already set by a rename and read by nobody, which is precisely that + * failure with the evidence discarded. A refused write leaves the local row + * exactly as it was, so what is on screen is what is on the server. + * + * Device-only lists are not this class's business: they have no href, no + * account and nothing to ask permission of. [de.jeanlucmakiola.agendula.data.tasks.TasksRepository] + * keeps them. + */ +@Singleton +class RemoteListRepository @Inject constructor( + private val database: TasksDatabase, + private val credentials: CredentialStore, + private val support: CollectionSupportStore, + private val syncTrigger: SyncTrigger, + private val cadence: SyncCadenceStore, + private val notices: SyncNoticeStore, + private val quarantine: QuarantineStore, + private val accountState: AccountStateStore, + private val resolver: ProviderResolver, + @IoDispatcher private val io: CoroutineDispatcher, +) { + + /** Why a collection write did not happen, in a form the UI can translate. */ + sealed interface Outcome { + data object Done : Outcome + + /** The server said no, and will say no again. */ + data class Refused(val code: Int) : Outcome + + /** The server could not be reached. Worth another try. */ + data object Unreachable : Outcome + + /** This account cannot make collections at all — iCloud, Posteo, Google. */ + data object Unsupported : Outcome + + /** Ours is a read-only share; the write belongs to whoever owns it. */ + data object ReadOnly : Outcome + + /** The account is gone, stopped, or has no credential we can decrypt. */ + data object NoAccount : Outcome + + /** + * The server answered something this call cannot make sense of. + * + * ⚠️ Not [Unreachable]. `CollectionOutcome` is one type across create, + * update and delete, so each of them has branches the other's method + * can return and its own cannot — and mapping those to [Unreachable] + * told someone sitting on wifi that they were offline. Unreachable is a + * claim about the network, and this is not one. + */ + data object Unexpected : Outcome + } + + /** + * The accounts a new list may be created on, freshest answer first. + * + * ⚠️ Re-asked rather than cached for ever. `CollectionSupportStore` holds + * the last answer so a picker can draw immediately, but a server that gained + * the capability in an upgrade — or lost it in a config change — must be + * able to say so, and the only moment that costs nothing is while the user + * is looking at the picker. + */ + suspend fun creatableAccounts(): List = withContext(io) { + // ⚠️ Empty in External mode, whatever the accounts table holds. The + // lists on screen then come from a third-party provider, so a row + // inserted into ours would exist, sync, and be visible to nobody. + if (resolver.mode() != StorageMode.OWN) return@withContext emptyList() + val stopped = accountState.needingSignIn() + val candidates = database.accounts().all().filter { + it.homeSetUrl?.toHttpUrlOrNull() != null && it.id !in stopped + } + // ⚠️ Together, not one after another. Each probe is a blocking OPTIONS, + // so three accounts with one server on a slow link held the "Where" row + // off the sheet for the sum of all three — with the sheet already drawn. + coroutineScope { + candidates.map { account -> async { account to supportFor(account) } } + .awaitAll() + .filter { (_, support) -> support.canCreate } + .map { (account, _) -> account } + } + } + + /** + * Makes a collection on [accountId]'s home set and a row pointing at it. + * + * @return the new list's local id, or why there is none. + */ + suspend fun create( + accountId: Long, + name: String, + color: Int, + ): Outcome = withContext(io) { + // ⚠️ Re-checked here, not only in `creatableAccounts`. The picker's list + // is a StateFlow that outlives one opening of the sheet, so a mode that + // flips between the list being built and Save being tapped would + // otherwise create the collection on the server and file the row in a + // store External mode never reads. + if (resolver.mode() != StorageMode.OWN) return@withContext Outcome.NoAccount + val account = database.accounts().account(accountId) ?: return@withContext Outcome.NoAccount + val homeSet = account.homeSetUrl?.toHttpUrlOrNull() + ?: return@withContext Outcome.NoAccount + val admin = adminFor(account) ?: return@withContext Outcome.NoAccount + val capabilities = support.refresh(accountId) { admin.support(homeSet) } + if (!capabilities.canCreate) return@withContext Outcome.Unsupported + + // ⚠️ A second attempt, but only for the one refusal a different name + // can fix. Nextcloud's trashbin *renames* a deleted collection rather + // than removing it, so re-creating under a segment used before answers + // 403 for ever — and "Shopping" is exactly the name someone deletes and + // remakes. Retrying anything else spends a second authenticated write + // that will fail the same way, and worse: a 401 is a second hit on the + // brute-force counter, and a 507 retried reports the wrong code back, + // since the caller only ever sees the *last* attempt's. + val first = ResourceNames.forCollection(name) + var created = admin.create(homeSet, first, name, color, capabilities) + if (created is CollectionOutcome.Refused && created.code in NAME_REFUSALS) { + created = admin.create(homeSet, ResourceNames.randomCollection(), name, color, capabilities) + } + + when (created) { + is CollectionOutcome.Created -> { + // ⚠️ Uncancellable. The collection exists on the server from + // here on, and a cancellation between that and the row would + // leave one the app has no record of and no way to reach — + // visible only on the next full account re-add. + withContext(NonCancellable) { + database.taskLists().insert( + TaskListEntity( + name = name, + color = color, + accountId = accountId, + href = created.url.toString(), + ), + ) + } + // The server has it and we do not; a sync is how the two agree + // on a ctag and a token rather than reconciling in full later. + syncTrigger.enqueue(account.displayName, expedited = true) + Outcome.Done + } + + is CollectionOutcome.Refused -> Outcome.Refused(created.code) + is CollectionOutcome.Failed -> Outcome.Unreachable + CollectionOutcome.Unsupported -> Outcome.Unsupported + CollectionOutcome.Updated -> Outcome.Unexpected + } + } + + /** + * Renames and recolours [listId] on the server, then locally. + * + * ⚠️ Refuses a read-only collection rather than discovering it at write + * time. A share the owner has made read-only answers 403 to a PROPPATCH, and + * a row that has already been renamed locally by then reads as a rename that + * worked and then quietly reverted on the next sync. + */ + suspend fun rename(listId: Long, name: String, color: Int): Outcome = withContext(io) { + val list = database.taskLists().entity(listId) ?: return@withContext Outcome.NoAccount + if (list.isReadOnly) return@withContext Outcome.ReadOnly + val url = list.href?.toHttpUrlOrNull() ?: return@withContext Outcome.NoAccount + val account = list.accountId?.let { database.accounts().account(it) } + ?: return@withContext Outcome.NoAccount + val admin = adminFor(account) ?: return@withContext Outcome.NoAccount + + when (val outcome = admin.updateProperties(url, displayName = name, color = color)) { + CollectionOutcome.Updated -> { + withContext(NonCancellable) { + // Read again inside the write: a sync running alongside this + // may have refreshed the ACL flag or the cursor, and writing + // back the entity we read before the network call would + // revert it. + val current = database.taskLists().entity(listId) ?: return@withContext + database.taskLists().update( + // isDirty stays false: the server already has this. The + // flag existed for a PROPPATCH that never happened. + current.copy(name = name, color = color, isDirty = false), + ) + } + Outcome.Done + } + + is CollectionOutcome.Refused -> Outcome.Refused(outcome.code) + is CollectionOutcome.Failed -> Outcome.Unreachable + is CollectionOutcome.Created, CollectionOutcome.Unsupported -> Outcome.Unexpected + } + } + + /** + * Deletes [listId] on the server, then on the device. + * + * ⚠️ The one write where "already gone" is success — [CollectionAdmin.delete] + * grades 404 and 410 that way — because otherwise a collection someone + * removed from another client leaves a row here that nothing can get rid of. + */ + suspend fun delete(listId: Long): Outcome = withContext(io) { + val list = database.taskLists().entity(listId) ?: return@withContext Outcome.Done + if (list.isReadOnly) return@withContext Outcome.ReadOnly + val url = list.href?.toHttpUrlOrNull() ?: return@withContext Outcome.NoAccount + val account = list.accountId?.let { database.accounts().account(it) } + ?: return@withContext Outcome.NoAccount + val admin = adminFor(account) ?: return@withContext Outcome.NoAccount + + when (val outcome = admin.delete(url)) { + CollectionOutcome.Updated -> { + withContext(NonCancellable) { + // `tasks.list_id` is ON DELETE CASCADE, so the tasks go with + // it — which is what was just done on the server. + database.taskLists().delete(listId) + // And the per-list state keyed off it, exactly as removing an + // account clears its lists': the ids are AUTOINCREMENT so + // nothing would ever read these again. The notices go by + // *name*, which is how they are keyed — a discarded-edit + // notice would otherwise name a list that no longer exists + // until the user tapped "Got it". + forgetPerListState(listId) + list.accountId?.let { notices.forgetList(it, list.name) } + } + Outcome.Done + } + + is CollectionOutcome.Refused -> Outcome.Refused(outcome.code) + is CollectionOutcome.Failed -> Outcome.Unreachable + is CollectionOutcome.Created, CollectionOutcome.Unsupported -> Outcome.Unexpected + } + } + + private suspend fun supportFor(account: AccountEntity): CollectionSupport { + val homeSet = account.homeSetUrl?.toHttpUrlOrNull() ?: return CollectionSupport.NONE + val admin = adminFor(account) ?: return CollectionSupport.NONE + return support.refresh(account.id) { admin.support(homeSet) } + } + + /** + * Null when the account has no credential we can use — a stopped account, or + * a restore. + * + * ⚠️ The stopped check is the same one `SyncEngine.sync` makes before it + * touches the network, and for the same reason: Nextcloud's brute-force + * protection throttles and then **429s per source IP**, so spending a + * request on a credential we already know the server rejects lands on the + * user's *other* clients. Opening the "new list" sheet must not do that any + * more than a timer may. + */ + private suspend fun adminFor(account: AccountEntity): CollectionAdmin? { + if (accountState.needsSignIn(account.id)) return null + val username = account.username ?: return null + val origin = account.principalUrl?.toHttpUrlOrNull() ?: return null + val password = (credentials.get(account.id) as? CredentialStore.Secret.Present)?.value + ?: return null + return DavCollectionAdmin(client(username, password, origin)) + } + + private fun client(username: String, password: String, origin: HttpUrl): OkHttpClient = + CalDavHttp.authenticated(USER_AGENT, username, password, origin) + + private suspend fun forgetPerListState(listId: Long) { + val ids = setOf(listId) + cadence.forget(ids) + quarantine.forget(ids) + } + + private companion object { + /** The same agent the sync and the add flow use, so the server names us once. */ + const val USER_AGENT = "Agendula (Android)" + + /** + * Refusals a different path segment can get past, and only those. + * + * 403 is Nextcloud's trashbin still holding the name; 405 is a server + * answering "already a collection there". Everything else — 401, 409, + * 423, 507 — means the same thing under any name. + */ + val NAME_REFUSALS = setOf(403, 405) + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncEngine.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncEngine.kt index 0348a6c..167282e 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncEngine.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncEngine.kt @@ -29,12 +29,20 @@ class SyncEngine @Inject constructor( private val quarantine: QuarantineStore, private val cadence: SyncCadenceStore, private val accountState: AccountStateStore, + private val notices: SyncNoticeStore, @IoDispatcher private val io: CoroutineDispatcher, ) { /** Why an account could not be synced at all, as opposed to one of its lists. */ sealed interface Result { - data class Synced(val reports: List) : Result + data class Synced( + val reports: List, + /** + * What this run destroyed or gave up on that was not already on + * record — the caller's cue to say so out loud. + */ + val notices: List = emptyList(), + ) : Result /** The credential is gone or undecryptable: only re-authentication helps. */ data class NeedsSignIn(val reason: String) : Result @@ -78,6 +86,18 @@ class SyncEngine @Inject constructor( val client = CalDavHttp.authenticated(USER_AGENT, username, password, origin) val reports = syncCollections(account) { url -> CalendarCollection(client, url) } + // ⚠️ Before the auth check, not after it. A 401 on one collection does + // not un-discard an edit another collection already destroyed, and + // returning NeedsSignIn past this point would drop the record of it. + // Outside `syncCollections` because that is driven without a network by + // the reconciliation tests, which have nothing to say about notices. + val fresh = notices.record( + accountId = account.id, + at = kotlin.time.Clock.System.now(), + reports = reports, + titles = quarantinedTitles(reports), + ) + if (reports.any { it.authFailure }) { // ⚠️ Stop the account rather than let the schedule keep trying. // Nextcloud throttles and then 429s **per source IP**, so a timer on a @@ -89,9 +109,29 @@ class SyncEngine @Inject constructor( } accountState.setNeedsSignIn(account.id, false) - Result.Synced(reports) + Result.Synced(reports, fresh) } + /** + * The local title of each quarantined resource, by href. + * + * ⚠️ Resolved here rather than left to the store, which has no database. + * Without it the user is told "a task has stopped syncing" over a row + * reading `a1f9c3e2-….ics` — the opaque blob `SyncNoticeStore` refuses to + * show for a discarded edit, and unactionable for exactly the same reason. + * A resource we have never stored has no title to find, and its filename is + * then genuinely all there is. + */ + private fun quarantinedTitles(reports: List): Map = + reports.filter { it.quarantined.isNotEmpty() } + .flatMap { report -> + val wanted = report.quarantined.mapTo(mutableSetOf()) { it.href } + store.rowsIn(report.listId) + .filter { it.href in wanted && !it.title.isNullOrBlank() } + .map { it.href!! to it.title!! } + } + .toMap() + /** * Records why the account could not be synced at all. * diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncNoticeNotifier.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncNoticeNotifier.kt new file mode 100644 index 0000000..fe15895 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncNoticeNotifier.kt @@ -0,0 +1,129 @@ +package de.jeanlucmakiola.agendula.data.sync + +import android.Manifest +import android.annotation.SuppressLint +import android.app.NotificationChannel +import android.app.NotificationManager +import android.app.PendingIntent +import android.content.Context +import android.content.pm.PackageManager +import android.os.Build +import androidx.core.app.NotificationCompat +import androidx.core.app.NotificationManagerCompat +import androidx.core.content.ContextCompat +import dagger.hilt.android.qualifiers.ApplicationContext +import de.jeanlucmakiola.agendula.MainActivity +import de.jeanlucmakiola.agendula.R +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Tells the user what a background sync destroyed or gave up on. + * + * ⚠️ A notification, and not only a row on the accounts screen. Sync runs on a + * four-hour timer while the app is closed, so a surface the user has to go and + * look at means the discarded edit is discovered — if ever — days later, next to + * a task that quietly says something else than what they typed. The account + * screen keeps the detail; this is what makes them go there. + * + * Its own channel, at `IMPORTANCE_LOW`: it is a report rather than an alarm, and + * it must be silenceable without taking due-task reminders with it. + */ +@Singleton +class SyncNoticeNotifier @Inject constructor( + @ApplicationContext private val context: Context, +) { + + fun canPost(): Boolean { + val granted = Build.VERSION.SDK_INT < Build.VERSION_CODES.TIRAMISU || + ContextCompat.checkSelfPermission(context, Manifest.permission.POST_NOTIFICATIONS) == + PackageManager.PERMISSION_GRANTED + return granted && NotificationManagerCompat.from(context).areNotificationsEnabled() + } + + // canPost() checks POST_NOTIFICATIONS before we ever call notify(). + @SuppressLint("MissingPermission") + fun post(accountName: String, notices: List) { + if (notices.isEmpty() || !canPost()) return + ensureChannel() + + val discarded = notices.count { it.kind == SyncNotice.Kind.DISCARDED_EDIT } + val quarantined = notices.size - discarded + // ⚠️ A discarded edit outranks a quarantine even when there are more + // quarantines, and the collapsed line says so. They are not equivalent: + // an edit that lost is work already destroyed and unrecoverable, while a + // quarantined task is a condition that persists and clears itself. The + // big text below lists both, in full, whichever headline was chosen. + val title = if (discarded > 0) { + context.resources.getQuantityString( + R.plurals.sync_notice_discarded_title, discarded, discarded, + ) + } else { + context.resources.getQuantityString( + R.plurals.sync_notice_quarantined_title, quarantined, quarantined, + ) + } + + val notification = NotificationCompat.Builder(context, CHANNEL_ID) + .setSmallIcon(R.drawable.ic_notification) + .setContentTitle(title) + .setContentText(context.getString(R.string.sync_notice_body, accountName)) + .setStyle(NotificationCompat.BigTextStyle().bigText(summaryOf(notices))) + .setCategory(NotificationCompat.CATEGORY_STATUS) + .setPriority(NotificationCompat.PRIORITY_LOW) + .setAutoCancel(true) + .setContentIntent( + PendingIntent.getActivity( + context, + accountName.hashCode(), + MainActivity.openIntent(context), + PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE, + ), + ) + .build() + + // Tagged by account, so a second account's news replaces nothing. + NotificationManagerCompat.from(context).notify(accountName, NOTIFICATION_ID, notification) + } + + /** + * The first few, by name. + * + * ⚠️ A count on its own is unactionable — "3 edits were replaced" leaves the + * user to guess which three, across every list they own. The names are the + * only part that makes the account screen worth opening. + */ + private fun summaryOf(notices: List): String { + val named = notices.take(SUMMARY_LIMIT).joinToString("\n") { notice -> + val subject = notice.subject.ifBlank { context.getString(R.string.task_untitled) } + context.getString(R.string.sync_notice_line, subject, notice.listName) + } + val rest = notices.size - SUMMARY_LIMIT + return if (rest > 0) { + named + "\n" + context.resources.getQuantityString(R.plurals.sync_notice_more, rest, rest) + } else { + named + } + } + + private fun ensureChannel() { + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) return + val manager = context.getSystemService(NotificationManager::class.java) + if (manager.getNotificationChannel(CHANNEL_ID) != null) return + manager.createNotificationChannel( + NotificationChannel( + CHANNEL_ID, + context.getString(R.string.sync_notice_channel_name), + NotificationManager.IMPORTANCE_LOW, + ).apply { description = context.getString(R.string.sync_notice_channel_desc) }, + ) + } + + private companion object { + const val CHANNEL_ID = "sync_notices" + const val NOTIFICATION_ID = 2 + + /** Enough to recognise the work; the screen has the rest. */ + const val SUMMARY_LIMIT = 5 + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncNoticeStore.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncNoticeStore.kt new file mode 100644 index 0000000..1ecf0bd --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncNoticeStore.kt @@ -0,0 +1,265 @@ +package de.jeanlucmakiola.agendula.data.sync + +import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.edit +import androidx.datastore.preferences.core.stringSetPreferencesKey +import de.jeanlucmakiola.agendula.data.di.SyncStateDataStore +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.flow.map +import java.util.Base64 +import javax.inject.Inject +import javax.inject.Singleton +import kotlin.time.Instant + +/** + * One thing a sync did that the user would not otherwise find out about. + * + * ⚠️ `docs/SYNC-PLAN.md` decision 2 is **server wins, local edit discarded**, and + * [SyncReport]'s own doc says the report is "the other half of the decision, not + * a nice-to-have". Until this existed the other half was a `Log.i` — the edit was + * gone, nothing in `ui/` read `discardedEdits`, and from where the user sits that + * is indistinguishable from the app losing their work. + */ +data class SyncNotice( + val accountId: Long, + val listName: String, + val kind: Kind, + /** The task's title for a discarded edit, the resource's name for a quarantine. */ + val subject: String, + /** + * What makes this notice distinct from another about a different task. + * + * ⚠️ Carried but never shown. These are stored as a `Set`, and two + * discarded edits from one run share an account, a list, a cause and a + * timestamp — so two *untitled* tasks, or two both called "Milk", encoded + * identically and one of them silently vanished. The notification counted + * two and the screen listed one. The UID is the only thing that tells them + * apart, and it is exactly what must not reach the user: opaque text chosen + * by whoever created the task. + */ + val key: String, + /** Why the edit lost. Null for a quarantine, which has no such choice behind it. */ + val cause: DiscardedEdit.Cause?, + val at: Instant, +) { + enum class Kind { DISCARDED_EDIT, QUARANTINED } +} + +/** + * What the last syncs destroyed or gave up on, per account, until it is read. + * + * The two kinds keep different company, which is why they are written + * differently: + * + * - A **discarded edit** is news. It happened once, it cannot be undone, and a + * later clean sync does not make it untrue — so it accumulates and is cleared + * only by the user acknowledging it. Replacing the set every run would let a + * quiet sync an hour later erase the one thing worth saying. + * - A **quarantined resource** is a standing condition: one task has stopped + * syncing while the rest of its list is fine. It is re-reported on every run + * for as long as it holds, so the account's set of them is *replaced* each + * time — which is also how it clears itself the moment the resource starts + * working again. + * + * Lives with the other per-device sync state, and is therefore excluded from + * backup — see [SyncStateDataStore]. Correct on its own terms too: a restored + * device has not discarded anything. + */ +@Singleton +class SyncNoticeStore @Inject constructor( + @SyncStateDataStore private val dataStore: DataStore, +) { + + /** Observed, so a background sync's news reaches a screen that is already open. */ + fun observeAll(): Flow> = dataStore.data.map { prefs -> + prefs[KEY].orEmpty().mapNotNull(::decode).sortedByDescending { it.at } + } + + /** + * Folds one account's run into the store. + * + * @return only what is **new**, which is what a notification may be posted + * for. A quarantine already on record is a condition the user has already + * been told about, and re-announcing it on every four-hour run would train + * them to ignore the one that matters. + */ + /** + * @param titles the local title of each quarantined resource, by href. + * ⚠️ Not optional decoration. Without it the row read + * `a1f9c3e2-….ics`, which is the opaque blob this file refuses to show + * for a discarded edit — and "a task has stopped syncing" that does not + * say which task is the very failure the feature exists to fix. Absent + * only for a resource we never stored, where the filename is genuinely + * all there is. + */ + suspend fun record( + accountId: Long, + at: Instant, + reports: List, + titles: Map = emptyMap(), + ): List { + val discarded = reports.flatMap { report -> + report.discardedEdits.map { edit -> + SyncNotice( + accountId = accountId, + listName = report.listName, + kind = SyncNotice.Kind.DISCARDED_EDIT, + // The UID is not shown to anyone: it is opaque text chosen by + // whoever created the task, routinely a bare hex blob. + subject = edit.title.orEmpty(), + key = edit.uid, + cause = edit.cause, + at = at, + ) + } + } + // ⚠️ Only the ones that have actually stopped. Below the threshold the + // resource is still being retried, and "one of your tasks has stopped + // syncing" would be untrue of a single 502 from a proxy mid-restart. + val quarantined = reports.flatMap { report -> + report.quarantined + .filter { it.failures >= QuarantineStore.THRESHOLD } + .map { resource -> + SyncNotice( + accountId = accountId, + listName = report.listName, + kind = SyncNotice.Kind.QUARANTINED, + subject = titles[resource.href] ?: resource.href.substringAfterLast('/'), + key = resource.href, + cause = null, + at = at, + ) + } + } + + // ⚠️ Nothing to say is the overwhelmingly common case — most syncs + // discard nothing and quarantine nothing — and a DataStore edit rewrites + // and fsyncs the whole file. Skipped only when there is also nothing on + // record to clear, or a recovered resource would keep its notice for ever. + if (discarded.isEmpty() && quarantined.isEmpty() && !hasRecord(accountId)) { + return emptyList() + } + + var added = emptyList() + dataStore.edit { prefs -> + // ⚠️ Re-read inside `edit`, which DataStore serialises. The set is + // global while `SyncWorker`'s uniqueness is only per account, so two + // accounts can be folding in at once and a snapshot taken outside + // would discard the other's. + val current = prefs[KEY].orEmpty().mapNotNull(::decode) + val others = current.filter { it.accountId != accountId } + val keptDiscards = current.filter { + it.accountId == accountId && it.kind == SyncNotice.Kind.DISCARDED_EDIT + } + val standing = current.filter { + it.accountId == accountId && it.kind == SyncNotice.Kind.QUARANTINED + } + added = discarded + quarantined.filterNot { fresh -> + standing.any { it.key == fresh.key } + } + // Newest first, then capped: an account that has been failing for a + // week must not grow this without bound, and the oldest news is the + // least actionable. + val kept = (discarded + keptDiscards).sortedByDescending { it.at }.take(MAX_PER_ACCOUNT) + // ⚠️ Capped as well, and the class doc used to claim it did not need + // to be. "Bounded by the collection" is only true of a healthy one: + // a server answering 415 to four hundred resources puts four hundred + // entries in one preference key, rewritten on every run — and the + // account screen renders them into a plain scrolling column. + val standingNow = quarantined.take(MAX_PER_ACCOUNT) + val updated = (others + kept + standingNow).map(::encode).toSet() + // ⚠️ Only when it differs. A DataStore edit rewrites and fsyncs the + // whole file, and an account holding one un-dismissed notice would + // otherwise pay that on every four-hour sync until the user tapped + // "Got it" — which is the cost the fast path above claims to avoid. + if (updated != prefs[KEY]) prefs[KEY] = updated + } + return added + } + + private suspend fun hasRecord(accountId: Long): Boolean = + dataStore.data.first().let { prefs -> + prefs[KEY].orEmpty().mapNotNull(::decode).any { it.accountId == accountId } + } + + /** + * Forgets one list's notices, for a list that has just been deleted. + * + * By name, because that is how they are keyed — there is no list id in a + * notice, and by the time this is called the row it would have named is + * already gone. + */ + suspend fun forgetList(accountId: Long, listName: String) { + dataStore.edit { prefs -> + val kept = prefs[KEY].orEmpty() + .mapNotNull(::decode) + .filterNot { it.accountId == accountId && it.listName == listName } + .map(::encode) + .toSet() + if (kept != prefs[KEY]) prefs[KEY] = kept + } + } + + /** The user has read them. */ + suspend fun dismiss(accountId: Long) { + dataStore.edit { prefs -> + prefs[KEY] = prefs[KEY].orEmpty() + .mapNotNull(::decode) + .filterNot { it.accountId == accountId } + .map(::encode) + .toSet() + } + } + + /** + * ⚠️ Base64 around the free text, not a delimiter and a hope. A list is + * named by its owner and a task is titled by its author, so both can hold + * any character at all — including whatever separator looked safe. + */ + private fun encode(notice: SyncNotice): String = listOf( + notice.accountId.toString(), + notice.kind.name, + notice.cause?.name.orEmpty(), + notice.at.toEpochMilliseconds().toString(), + base64(notice.listName), + base64(notice.subject), + base64(notice.key), + ).joinToString(SEPARATOR) + + private fun decode(entry: String): SyncNotice? { + val parts = entry.split(SEPARATOR) + if (parts.size != FIELDS) return null + val accountId = parts[0].toLongOrNull() ?: return null + val kind = SyncNotice.Kind.entries.firstOrNull { it.name == parts[1] } ?: return null + val at = parts[3].toLongOrNull() ?: return null + return SyncNotice( + accountId = accountId, + listName = unBase64(parts[4]) ?: return null, + kind = kind, + subject = unBase64(parts[5]) ?: return null, + key = unBase64(parts[6]) ?: return null, + cause = DiscardedEdit.Cause.entries.firstOrNull { it.name == parts[2] }, + at = Instant.fromEpochMilliseconds(at), + ) + } + + private fun base64(value: String): String = + Base64.getUrlEncoder().encodeToString(value.toByteArray(Charsets.UTF_8)) + + private fun unBase64(value: String): String? = runCatching { + String(Base64.getUrlDecoder().decode(value), Charsets.UTF_8) + }.getOrNull() + + private companion object { + val KEY = stringSetPreferencesKey("sync_notices") + + /** Not present in URL-safe Base64, nor in a decimal or an enum name. */ + const val SEPARATOR = "|" + const val FIELDS = 7 + + /** Discarded edits, per account. Quarantines are bounded by the collection. */ + const val MAX_PER_ACCOUNT = 50 + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncWorker.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncWorker.kt index 04a1a8e..c774f91 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncWorker.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncWorker.kt @@ -32,6 +32,7 @@ class SyncWorker @AssistedInject constructor( @Assisted context: Context, @Assisted parameters: WorkerParameters, private val engine: SyncEngine, + private val noticeNotifier: SyncNoticeNotifier, ) : CoroutineWorker(context, parameters) { override suspend fun doWork(): Result { @@ -46,6 +47,12 @@ class SyncWorker @AssistedInject constructor( outcome.reports.forEach { report -> if (report.failure != null || report.hadWork) Log.i(TAG, report.toString()) } + // ⚠️ Said out loud, not only logged. `SyncReport`'s own doc + // calls the report "the other half" of server-wins, and this + // worker runs on a four-hour timer with the app closed — so a + // log line is the same as saying nothing. Only what is new: the + // store has already dropped whatever the user has been told. + noticeNotifier.post(accountName, outcome.notices) Result.success() } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/Entities.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/Entities.kt index ba58e5b..ae96886 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/Entities.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/Entities.kt @@ -66,6 +66,15 @@ data class TaskListEntity( @ColumnInfo(name = "ctag") val ctag: String? = null, /** RFC 6578 sync token, per collection. */ @ColumnInfo(name = "sync_token") val syncToken: String? = null, + /** + * Unused, and kept only because dropping a column costs a migration. + * + * ⚠️ It was set by a rename and read by nobody. A collection's name and + * colour are now written server-first by + * [de.jeanlucmakiola.agendula.data.sync.RemoteListRepository], so there is + * no local edit left waiting to be pushed — and a flag that means "owed to + * the server" while nothing ever pays it is worse than no flag at all. + */ @ColumnInfo(name = "is_dirty", defaultValue = "0") val isDirty: Boolean = false, ) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTaskMapper.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTaskMapper.kt index 32c24eb..921d581 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTaskMapper.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTaskMapper.kt @@ -25,6 +25,8 @@ object RoomTaskMapper { isSynced = row.list.isSynced, isVisible = row.list.isVisible, owner = row.list.owner, + accountId = row.list.accountId, + isReadOnly = row.list.isReadOnly, ) /** diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSource.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSource.kt index 351f5db..2b0fc16 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSource.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/RoomTasksDataSource.kt @@ -297,17 +297,19 @@ class RoomTasksDataSource @Inject constructor( override fun createLocalList(name: String, color: Int): Long = lists.insert(TaskListEntity(name = name.trim(), color = color)) + /** + * ⚠️ Device-only lists. An account-backed collection is renamed through + * [de.jeanlucmakiola.agendula.data.sync.RemoteListRepository], which sends + * the PROPPATCH first and writes here only once the server has taken it. + * + * This used to set `is_dirty` for an account list instead, on the theory + * that a later sync would push it. Nothing ever read that flag: the rename + * never left the phone, and the next run that re-read the collection's + * `displayname` quietly put the old name back. + */ override fun updateList(listId: Long, name: String, color: Int) { val current = lists.entity(listId) ?: throw TaskWriteFailedException("update list $listId") - // Only an account-backed collection owes a server a PROPPATCH; a - // device-only list has nothing to be dirty for. - lists.update( - current.copy( - name = name.trim(), - color = color, - isDirty = current.accountId != null, - ), - ) + lists.update(current.copy(name = name.trim(), color = color)) } /** `tasks.list_id` is `ON DELETE CASCADE`, so the list's tasks go with it. */ diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/domain/Models.kt b/app/src/main/java/de/jeanlucmakiola/agendula/domain/Models.kt index 5f16df4..b446da1 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/domain/Models.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/domain/Models.kt @@ -12,6 +12,19 @@ data class TaskList( val isSynced: Boolean, val isVisible: Boolean, val owner: String?, + /** + * The CalDAV account this list belongs to, or null for a device-only one. + * + * ⚠️ Distinct from [accountName], which is a label and is filled in with a + * placeholder for a device-only list. This is what a write has to be aimed + * at, so it has to be able to say "nowhere". + */ + val accountId: Long? = null, + /** + * A share we may read and not write. Renaming or deleting it is the owner's + * to do, and attempting either answers 403. + */ + val isReadOnly: Boolean = false, ) { /** A device-only list Agendula (or another app) created locally, not synced. */ val isLocal: Boolean diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountDetailScreen.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountDetailScreen.kt index 21d408e..c3b63d3 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountDetailScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountDetailScreen.kt @@ -7,10 +7,13 @@ import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.padding import androidx.compose.material.icons.Icons import androidx.compose.material.icons.automirrored.rounded.Login +import androidx.compose.material.icons.rounded.Check import androidx.compose.material.icons.rounded.CloudSync import androidx.compose.material.icons.rounded.DeleteOutline import androidx.compose.material.icons.rounded.History +import androidx.compose.material.icons.rounded.HistoryToggleOff import androidx.compose.material.icons.rounded.Inventory2 +import androidx.compose.material.icons.rounded.SyncProblem import androidx.compose.material3.Icon import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text @@ -30,6 +33,8 @@ import androidx.compose.ui.text.withStyle import androidx.compose.ui.unit.dp import androidx.lifecycle.compose.collectAsStateWithLifecycle import de.jeanlucmakiola.agendula.R +import de.jeanlucmakiola.agendula.data.sync.DiscardedEdit +import de.jeanlucmakiola.agendula.data.sync.SyncNotice import de.jeanlucmakiola.floret.components.CollapsingScaffold import de.jeanlucmakiola.floret.components.FullScreenPicker import de.jeanlucmakiola.floret.components.GroupedListInset @@ -64,7 +69,9 @@ internal fun AccountDetailScreen( val account = shown.account val identity = account.identity() - var removing by remember { mutableStateOf(false) } + // The confirmation sheet. The removal it starts outlives this screen, and is + // shown on the accounts list rather than here. + var confirming by remember { mutableStateOf(false) } CollapsingScaffold(title = identity.title, onBack = onBack) { AccountHero(identity) @@ -101,9 +108,28 @@ internal fun AccountDetailScreen( } actions.forEachIndexed { index, action -> action(positionOf(index, actions.size)) } + // ⚠️ Above the removal, below the state — because it is the one thing on + // this screen the user did not already know. `docs/SYNC-PLAN.md` + // decision 2 is server-wins, and this group is the other half of it: + // without it a discarded edit is indistinguishable from lost work. + if (shown.notices.isNotEmpty()) { + Spacer(Modifier.height(24.dp)) + SyncNotices( + notices = shown.notices, + onDismiss = { viewModel.dismissNotices(account) }, + ) + } + Spacer(Modifier.height(24.dp)) // Its own group, away from the things that are safe to press twice. + // + // ⚠️ Nothing here shows the removal in flight, deliberately. Confirming + // hands off to `onRemoved`, which puts this screen away in the same + // frame — so a pending state drawn here could never appear, and the + // dimming and spinner that were written for it were dead code claiming + // to prevent a second tap that cannot happen. The wait is visible where + // the user actually ends up: the row on `AccountsScreen`. GroupedRow( title = errorTitle(stringResource(R.string.accounts_remove)), position = Position.Alone, @@ -114,24 +140,87 @@ internal fun AccountDetailScreen( tint = MaterialTheme.colorScheme.error, ) }, - onClick = { removing = true }, + onClick = { confirming = true }, ) Spacer(Modifier.height(24.dp)) } - if (removing) { + if (confirming) { RemoveAccountPicker( identity = identity, - onDismiss = { removing = false }, + onDismiss = { confirming = false }, onRemove = { deleteLocalData -> viewModel.remove(account, deleteLocalData) - removing = false + confirming = false onRemoved() }, ) } } +/** + * What the last syncs replaced or gave up on, one row each. + * + * Named, never counted. "3 edits were replaced" leaves the user to work out + * which three across every list they own, which is the same as not telling them + * — so each row carries the task's own title, the list it lives in, and the + * reason, and "Got it" is what clears them. + */ +@Composable +private fun SyncNotices(notices: List, onDismiss: () -> Unit) { + Text( + stringResource(R.string.sync_notices_title), + style = MaterialTheme.typography.labelMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + modifier = Modifier.padding(horizontal = GroupedListInset, vertical = 8.dp), + ) + // The dismissal is a row of the same group: it is the last thing you do to + // this list, and a floating button beside it would read as belonging to the + // screen rather than to these notices. + val rows = notices.size + 1 + notices.forEachIndexed { index, notice -> + GroupedRow( + title = notice.subject.ifBlank { stringResource(R.string.task_untitled) }, + summary = stringResource( + R.string.accounts_summary, + notice.listName, + stringResource(notice.cause.message), + ), + position = positionOf(index, rows), + leading = { + Icon( + if (notice.kind == SyncNotice.Kind.QUARANTINED) { + Icons.Rounded.SyncProblem + } else { + Icons.Rounded.HistoryToggleOff + }, + contentDescription = null, + ) + }, + ) + } + GroupedRow( + title = stringResource(R.string.sync_notices_dismiss), + position = positionOf(notices.size, rows), + leading = { Icon(Icons.Rounded.Check, contentDescription = null) }, + onClick = onDismiss, + ) +} + +/** + * Why an edit is gone, in words rather than an enum name. + * + * A quarantine has no cause of its own — nothing chose it, a resource simply + * kept failing — so the null branch is the sentence for that, not a fallback. + */ +private val DiscardedEdit.Cause?.message: Int + get() = when (this) { + DiscardedEdit.Cause.SERVER_NEWER -> R.string.sync_notice_cause_server_newer + DiscardedEdit.Cause.DELETED_ON_SERVER -> R.string.sync_notice_cause_deleted_on_server + DiscardedEdit.Cause.DELETE_LOST -> R.string.sync_notice_cause_delete_lost + null -> R.string.sync_notice_cause_quarantined + } + /** The account's logo at full size, over the two things the title bar left out. */ @Composable private fun AccountHero(identity: AccountIdentity) { diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt index b9133b5..a1efcba 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt @@ -4,11 +4,13 @@ import android.text.format.DateUtils import androidx.compose.foundation.layout.Spacer import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.size import androidx.compose.material.icons.Icons import androidx.compose.material.icons.automirrored.rounded.Login import androidx.compose.material.icons.rounded.Add import androidx.compose.material.icons.rounded.CloudSync import androidx.compose.material.icons.rounded.Sync +import androidx.compose.material3.CircularProgressIndicator import androidx.compose.material3.Icon import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme @@ -16,6 +18,7 @@ import androidx.compose.material3.Text import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue import androidx.compose.ui.Modifier +import androidx.compose.ui.res.pluralStringResource import androidx.compose.ui.res.stringResource import androidx.compose.ui.unit.dp import androidx.lifecycle.compose.collectAsStateWithLifecycle @@ -68,19 +71,39 @@ internal fun AccountsScreen( val identity = account.identity() GroupedRow( title = identity.title, - summary = accountSummary(identity.user, syncState(row)), + // One line, so it says the most useful thing it can. A row + // on its way out has one piece of news; an account holding + // unread sync reports has another, and either outranks + // "synced 5 minutes ago" — but neither outranks a sign-in + // that has stopped, which `syncState` already puts first. + summary = when { + row.removing -> stringResource(R.string.accounts_removing) + row.notices.isNotEmpty() && !row.needsSignIn -> accountSummary( + identity.user, + pluralStringResource( + R.plurals.accounts_notices, + row.notices.size, + row.notices.size, + ), + ) + else -> accountSummary(identity.user, syncState(row)) + }, position = positionOf(index, loaded.size), + // A row whose account is on its way out is not a row you can + // act on, so it stops looking like one: dimmed, its action + // replaced by the progress, and nothing to tap. + dimmed = row.removing, leading = { ProviderLogo(identity.provider) }, trailing = { - if (row.needsSignIn) { - IconButton(onClick = onAddAccount) { + when { + row.removing -> CircularProgressIndicator(Modifier.size(20.dp)) + row.needsSignIn -> IconButton(onClick = onAddAccount) { Icon( Icons.AutoMirrored.Rounded.Login, contentDescription = stringResource(R.string.accounts_sign_in_again), ) } - } else { - IconButton(onClick = { viewModel.syncNow(account) }) { + else -> IconButton(onClick = { viewModel.syncNow(account) }) { Icon( Icons.Rounded.Sync, contentDescription = stringResource(R.string.accounts_sync_now), @@ -88,7 +111,7 @@ internal fun AccountsScreen( } } }, - onClick = { onOpenAccount(account.id) }, + onClick = { onOpenAccount(account.id) }.takeUnless { row.removing }, ) } Spacer(Modifier.height(24.dp)) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt index f52a44b..5cb0f35 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt @@ -5,12 +5,16 @@ import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel import de.jeanlucmakiola.agendula.data.sync.AccountRepository import de.jeanlucmakiola.agendula.data.sync.AccountStateStore +import de.jeanlucmakiola.agendula.data.sync.SyncNotice +import de.jeanlucmakiola.agendula.data.sync.SyncNoticeStore import de.jeanlucmakiola.agendula.data.sync.SyncTrigger import de.jeanlucmakiola.agendula.data.tasks.room.AccountEntity +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.stateIn +import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import javax.inject.Inject @@ -19,10 +23,43 @@ class AccountsViewModel @Inject constructor( private val repository: AccountRepository, private val syncTrigger: SyncTrigger, private val accountState: AccountStateStore, + private val notices: SyncNoticeStore, ) : ViewModel() { - /** An account row, plus the one thing the row cannot read from the entity. */ - data class AccountRow(val account: AccountEntity, val needsSignIn: Boolean) + /** An account row, plus the two things the row cannot read from the entity. */ + data class AccountRow( + val account: AccountEntity, + val needsSignIn: Boolean, + /** + * A removal is under way for this account. + * + * The row survives it: `remove()` revokes the app password over the + * network before it touches any store, so several seconds pass — on a + * server that stalls, longer — during which the row sat there looking + * completely untouched. + */ + val removing: Boolean = false, + /** + * What the last syncs destroyed or gave up on, newest first. + * + * ⚠️ On the row rather than in a flow of its own, because it has to + * survive the account it belongs to going away: an account is removed + * by id, and a notice keyed to an id nothing lists any more is a line + * of text about nothing. + */ + val notices: List = emptyList(), + ) + + /** + * Accounts whose removal has started and not finished. + * + * ⚠️ Also the double-tap guard. Nothing else stops a second Remove from + * starting a second revocation of a credential the first one is already + * handing back — harmless since `c2166b9`, because the sequence is + * idempotent either way, but it spends a second network round trip and + * leaves the user watching two things happen to one account. + */ + private val _removing = MutableStateFlow>(emptySet()) /** * `null` until the first load, so the empty state does not flash. @@ -37,8 +74,18 @@ class AccountsViewModel @Inject constructor( combine( repository.observeAll(), accountState.observeNeedingSignIn(), - ) { accounts, stopped -> - accounts.map { AccountRow(it, it.id in stopped) } + _removing, + notices.observeAll(), + ) { accounts, stopped, removing, allNotices -> + val byAccount = allNotices.groupBy { it.accountId } + accounts.map { + AccountRow( + account = it, + needsSignIn = it.id in stopped, + removing = it.id in removing, + notices = byAccount[it.id].orEmpty(), + ) + } }.stateIn( scope = viewModelScope, started = SharingStarted.WhileSubscribed(STOP_TIMEOUT_MILLIS), @@ -59,10 +106,28 @@ class AccountsViewModel @Inject constructor( syncTrigger.enqueue(account.displayName, expedited = true) } + /** The user has read what the last sync changed. */ + fun dismissNotices(account: AccountEntity) { + viewModelScope.launch { notices.dismiss(account.id) } + } + fun remove(account: AccountEntity, deleteLocalData: Boolean) { + if (account.id in _removing.value) return + _removing.update { it + account.id } // No refresh: the row disappears because the query behind `accounts` // re-emits. - viewModelScope.launch { repository.remove(account.id, account.displayName, deleteLocalData) } + viewModelScope.launch { + try { + repository.remove(account.id, account.displayName, deleteLocalData) + } finally { + // ⚠️ In a `finally`, and not only on the happy path. `remove()` + // finishes its destructive tail uncancellable, so the id would + // otherwise be stranded in the set by the one case that reaches + // here without completing normally — leaving a row that is gone + // from Room but pending for ever if it ever came back. + _removing.update { it - account.id } + } + } } private companion object { diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountScreen.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountScreen.kt index f64d4b2..028d3f4 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountScreen.kt @@ -19,6 +19,7 @@ import androidx.compose.material.icons.rounded.CloudOff import androidx.compose.material.icons.rounded.CloudSync import androidx.compose.material.icons.rounded.Lock import androidx.compose.material.icons.rounded.OpenInBrowser +import androidx.compose.material.icons.rounded.Warning import androidx.compose.material3.Icon import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme @@ -177,6 +178,7 @@ internal fun AddAccountScreen( is AddAccountStep.EnterAddress -> AddressStep(step, viewModel) is AddAccountStep.Working -> WorkingStep(step) is AddAccountStep.EnterCredentials -> CredentialsStep(step, viewModel) + is AddAccountStep.ConfirmBrowser -> ConfirmBrowserStep(step) is AddAccountStep.WaitingForBrowser -> BrowserStep(step) is AddAccountStep.ChooseLists -> ListsStep(step, viewModel) is AddAccountStep.Summary -> SummaryStep(step) @@ -227,9 +229,13 @@ private fun AddAccountStep.position(hasSetup: Boolean): Int? { is AddAccountStep.ChooseProvider -> 1 is AddAccountStep.PrepareAccess -> 2 is AddAccountStep.EnterAddress -> 2 + errand - // One slot, because only one of the two ever happens: a server either - // hands the sign-in to a browser or asks for a password. - is AddAccountStep.EnterCredentials, is AddAccountStep.WaitingForBrowser -> 3 + errand + // One slot, because only one of the three ever happens: a server either + // hands the sign-in to a browser — asking first, where the URL needs + // confirming — or asks for a password. + is AddAccountStep.EnterCredentials, + is AddAccountStep.ConfirmBrowser, + is AddAccountStep.WaitingForBrowser, + -> 3 + errand is AddAccountStep.ChooseLists -> 4 + errand // The receipt keeps the bar full rather than dropping it: the flow is // finished, and a chrome that vanishes on the last screen reads as a @@ -263,6 +269,9 @@ private fun StepHero(state: AddAccountUiState) { val icon = when { state.fatal != null -> Icons.Rounded.CloudOff state.step is AddAccountStep.EnterCredentials -> Icons.Rounded.Lock + // The warning is the screen, so it is the mark too — the browser icon + // would say "this is going fine", which is the opposite of the point. + state.step is AddAccountStep.ConfirmBrowser -> Icons.Rounded.Warning state.step is AddAccountStep.WaitingForBrowser -> Icons.Rounded.OpenInBrowser state.step is AddAccountStep.ChooseLists -> Icons.Rounded.Checklist else -> Icons.Rounded.CloudSync @@ -298,6 +307,8 @@ private fun StepTitle(step: AddAccountStep) { } is AddAccountStep.EnterCredentials -> R.string.add_account_credentials_title to R.string.add_account_credentials_body + is AddAccountStep.ConfirmBrowser -> + R.string.add_account_browser_confirm_title to R.string.add_account_browser_confirm_body is AddAccountStep.ChooseLists -> R.string.add_account_lists_title to R.string.add_account_lists_body is AddAccountStep.Summary -> @@ -383,6 +394,22 @@ private fun ColumnScope.StepActions(state: AddAccountUiState, viewModel: AddAcco onClick = viewModel::onSummaryDone, ) + // The way forward stays primary: both causes are legitimate behind a + // reverse proxy, and refusing outright would make Login Flow v2 unusable + // for a large share of self-hosted installs. The password route is the + // secondary, exactly as it is one step later. + is AddAccountStep.ConfirmBrowser -> { + PrimaryAction( + label = stringResource(R.string.add_account_browser_open), + enabled = true, + onClick = viewModel::onBrowserConfirmed, + ) + TextButton( + onClick = viewModel::onBrowserCancelled, + modifier = Modifier.fillMaxWidth(), + ) { Text(stringResource(R.string.add_account_browser_use_password)) } + } + is AddAccountStep.WaitingForBrowser -> { // A failed flow gets the primary action: there is nothing left to // wait for, and the way forward is a password. diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModel.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModel.kt index 669935a..fb28caf 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModel.kt @@ -1,5 +1,6 @@ package de.jeanlucmakiola.agendula.ui.accounts.add +import androidx.lifecycle.SavedStateHandle import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel @@ -87,6 +88,29 @@ sealed interface AddAccountStep { val error: AddAccountMessage? = null, ) : AddAccountStep + /** + * The flow has a login URL, and something about it has to be read *before* + * the browser opens. + * + * ⚠️ Its own step, not a note on [WaitingForBrowser]. The note and the + * launch used to arrive in the same state update, so the Custom Tab was + * already covering the screen by the time the warning drew — and + * [NextcloudLoginFlow.Flow]'s own doc claims "the UI confirms it with the + * user, quoting the cause". That page is where the **account** password is + * typed, so the confirmation has to come first or it is not a confirmation. + * + * Two things get here. A [hostMismatch] means the server sent the sign-in + * somewhere other than the address that was typed — legitimate behind a + * reverse proxy, which is why it is confirmed rather than refused. An + * [insecure] login URL means the password would travel in the clear: + * `requireSecureOrigin` cannot fire for it, because a typed `http://` base + * makes a cleartext login URL consistent rather than a downgrade. + */ + data class ConfirmBrowser( + val hostMismatch: NextcloudLoginFlow.HostMismatch? = null, + val insecure: Boolean = false, + ) : AddAccountStep + /** * The browser has the flow. We poll until the user approves, and offer an * explicit way out — Custom Tabs return **no result** when dismissed, and @@ -165,32 +189,123 @@ class AddAccountViewModel @Inject constructor( private val repository: AccountCreator, private val gateway: CalDavGateway, private val pendingFlow: LoginFlowRecord, + /** + * Where the wizard's own state lives, so process death is not the end of it. + * + * ⚠️ Every field below used to be a plain `var`, and the browser step makes + * process death ordinary rather than exotic: the Custom Tab is a separate + * task, so ours is a background process while the user approves, and the + * system kills those. [PendingLoginFlowStore] closed the half of that window + * *before* approval — a poll token can be reclaimed and the password revoked. + * This closes the half after it, where an already-minted one-shot password + * existed only in memory. + * + * ⚠️ What goes in here is deliberately not "everything". A password the user + * **typed** is not saved: it is their account password on the generic CalDAV + * route, retyping it costs one field, and saved state is written out by the + * system. A password the server **minted** is saved, because it is a + * device-scoped app password that cannot be re-obtained — the 200 that + * carried it is spent — and losing it leaves a live credential in the user's + * device list under the same name as every other attempt, which is exactly + * what they cannot tell apart and so dare not prune. Saved state dies with + * the task, so unlike a store of our own it cannot outlive the flow. + */ + private val handle: SavedStateHandle, // ⚠️ Not viewModelScope. Handing the password back has to survive the // ViewModel that minted it, and androidx closes viewModelScope *before* // onCleared runs, so a launch there never executes its body. @ApplicationScope private val appScope: CoroutineScope, ) : ViewModel() { - private val _state = MutableStateFlow(AddAccountUiState()) - val state: StateFlow = _state.asStateFlow() + private val mutableState = MutableStateFlow(AddAccountUiState()) + val state: StateFlow = mutableState.asStateFlow() - private var choice: ProviderChoice? = null + /** + * The one write path for [state], so that [record] cannot be forgotten at + * one of the twenty-odd places the flow changes step. + * + * ⚠️ Synchronous, not a collector on [state]. The system asks for saved + * state on the main thread, and a recorder that only runs when the + * dispatcher next gets a turn is a recorder that loses whatever happened + * immediately before the kill — which is precisely the moment worth saving. + */ + private var current: AddAccountUiState + get() = mutableState.value + set(value) { + mutableState.value = value + record(value) + } + + private fun updateState(transform: (AddAccountUiState) -> AddAccountUiState) { + current = transform(current) + } + + private var choice: ProviderChoice? + get() = handle.get(KEY_CHOICE)?.let(::choiceOf) + set(value) { + handle[KEY_CHOICE] = value?.let { + when (it) { + is ProviderChoice.Service -> it.provider.name + ProviderChoice.OtherServer -> OTHER_SERVER_TOKEN + } + } + } /** * What is in the address field, as opposed to [typedInput], which is what was * last *submitted*. Kept so stepping back to the picker and forward again * returns to a filled field rather than an empty one. */ - private var addressInput: String = "" - private var typedInput: String = "" - private var serverRoot: HttpUrl? = null - private var username: String = "" + private var addressInput: String + get() = handle[KEY_ADDRESS_INPUT] ?: "" + set(value) { handle[KEY_ADDRESS_INPUT] = value } + + private var typedInput: String + get() = handle[KEY_TYPED_INPUT] ?: "" + set(value) { handle[KEY_TYPED_INPUT] = value } + + private var serverRoot: HttpUrl? + get() = handle.get(KEY_SERVER_ROOT)?.toHttpUrlOrNull() + set(value) { handle[KEY_SERVER_ROOT] = value?.toString() } + + private var username: String + get() = handle[KEY_USERNAME] ?: "" + set(value) { handle[KEY_USERNAME] = value } + + /** + * ⚠️ Not saved, unlike everything around it. On the browser route this holds + * a minted app password, which [minted] saves for its own reasons; on the + * generic route it holds the password the user typed, and that is theirs + * rather than ours to write down. A restore that has a minted credential + * refills this from it, and one that does not lands on a field to type into. + */ private var appPassword: String = "" + + /** + * ⚠️ Deliberately **not** saved, and re-derived instead. It carries a + * principal, several home sets and every collection the server listed, none + * of which is small and all of which the server will answer for again; a + * restore that needs it re-runs discovery with the credential we still hold. + * That also re-reads an ACL or a display name the server changed meanwhile, + * which a serialised snapshot could not. + */ private var found: CalDavDiscovery.Outcome.Found? = null private var pollJob: Job? = null + /** + * A started flow waiting on the user's say-so, held for the length of + * [AddAccountStep.ConfirmBrowser]. + * + * Nothing has been written down for it and nothing has been minted, so + * dropping it is a complete exit: the server keeps a row nobody will ever + * approve, and it expires on its own in twenty minutes. + */ + private var unconfirmedFlow: NextcloudLoginFlow.Flow? = null + /** Which hosts asked for credentials, for the cross-domain diagnostic. */ - private var hostsNeedingAuth: List = emptyList() + private var hostsNeedingAuth: List + get() = handle.get>(KEY_HOSTS_NEEDING_AUTH).orEmpty() + set(value) { handle[KEY_HOSTS_NEEDING_AUTH] = ArrayList(value) } /** * A password the browser flow minted and nothing owns yet. @@ -200,8 +315,29 @@ class AddAccountViewModel @Inject constructor( * under the same name as every other attempt, so they cannot tell which one * their working account uses and dare not prune any. Cleared without * revoking only when [AccountCreator] takes ownership. + * + * ⚠️ The one secret that *is* saved, and the reason [handle] exists at all — + * see its doc for why this and not the typed password. */ - private var minted: CalDavGateway.Credentials? = null + private var minted: CalDavGateway.Credentials? + get() { + val password = handle.get(KEY_MINTED_PASSWORD) ?: return null + val origin = handle.get(KEY_MINTED_ORIGIN)?.toHttpUrlOrNull() ?: return null + return CalDavGateway.Credentials( + username = handle[KEY_MINTED_USERNAME] ?: "", + password = password, + origin = origin, + ) + } + set(value) { + handle[KEY_MINTED_USERNAME] = value?.username + handle[KEY_MINTED_PASSWORD] = value?.password + handle[KEY_MINTED_ORIGIN] = value?.origin?.toString() + } + + init { + restore() + } /** * Step 1 → step 2, on the tap itself. @@ -221,7 +357,7 @@ class AddAccountViewModel @Inject constructor( choice = picked val quirk = picked.provider?.let(ServerQuirk::forProvider) val setup = quirk?.hasSetupSteps == true - _state.update { + updateState { it.copy( step = when { quirk?.isFatal == true -> AddAccountStep.ChooseProvider(picked) @@ -239,8 +375,8 @@ class AddAccountViewModel @Inject constructor( /** The errand is read; on to the address. */ fun onSetupAcknowledged() { - if (_state.value.step !is AddAccountStep.PrepareAccess) return - _state.update { it.copy(step = addressStep(addressInput), fatal = null) } + if (current.step !is AddAccountStep.PrepareAccess) return + updateState { it.copy(step = addressStep(addressInput), fatal = null) } } fun onAddressChanged(value: String) { @@ -250,7 +386,7 @@ class AddAccountViewModel @Inject constructor( val picked = choice ?: ProviderChoice.OtherServer choice = picked addressInput = value - _state.update { + updateState { it.copy( step = AddAccountStep.EnterAddress(picked, value), // The service's own rule outranks the one the typed host implies: @@ -267,7 +403,7 @@ class AddAccountViewModel @Inject constructor( /** Step 2 → discovery, unauthenticated. */ fun onAddressSubmitted() { - val input = (_state.value.step as? AddAccountStep.EnterAddress)?.input?.trim().orEmpty() + val input = (current.step as? AddAccountStep.EnterAddress)?.input?.trim().orEmpty() if (input.isEmpty()) return // ⚠️ The retry path, and the one that actually leaks. A post-approval // failure sends the user back here with an error, and the screen offers @@ -286,10 +422,12 @@ class AddAccountViewModel @Inject constructor( // host list the diagnostics read. found = null hostsNeedingAuth = emptyList() + // As does a flow started for it and never opened. + unconfirmedFlow = null // The mismatch is sticky on purpose, but it belongs to the address that // produced it — carrying it into a different server's flow makes a claim // about that server's settings which was never measured. - _state.update { it.copy(originMismatch = null) } + updateState { it.copy(originMismatch = null) } typedInput = input addressInput = input @@ -297,7 +435,7 @@ class AddAccountViewModel @Inject constructor( if (quirk?.isFatal == true) { // Google supports neither VTODO nor MKCALENDAR. Refusing with an // explanation beats a 401 the user cannot act on. - _state.update { + updateState { it.copy( step = addressStep(input), fatal = AddAccountMessage.GoogleUnsupported, @@ -334,7 +472,7 @@ class AddAccountViewModel @Inject constructor( /** Step 3a → re-run discovery with the password the user typed. */ fun onCredentialsSubmitted() { - val step = _state.value.step as? AddAccountStep.EnterCredentials ?: return + val step = current.step as? AddAccountStep.EnterCredentials ?: return if (step.username.isBlank() || step.password.isEmpty()) return username = step.username.trim() appPassword = step.password @@ -375,7 +513,7 @@ class AddAccountViewModel @Inject constructor( } } - private fun credentialsRejected() = _state.update { + private fun credentialsRejected() = updateState { it.copy( step = AddAccountStep.EnterCredentials( username = username, @@ -394,23 +532,58 @@ class AddAccountViewModel @Inject constructor( if (flow == null) { // Not a Nextcloud, or its login flow is unavailable. Ask for a // username and password instead — that is the generic CalDAV path. - _state.update { + updateState { it.copy(step = AddAccountStep.EnterCredentials(error = quirkHint())) } return } - // ⚠️ Written down **before** the browser is offered the URL, which is - // what the flow's own doc asks for: the browser is a separate task, so - // dying while the user approves is ordinary, and the poll token is the - // only way back to the password the server is about to mint. - // - // ⚠️ Guarded: a DataStore write can throw, and this runs in - // viewModelScope, where an escaping exception takes the app down. A - // flow we failed to write down is a reclaim we will not get, not a - // reason to lose the sign-in in front of the user. + // ⚠️ Confirmed before the browser, never alongside it. Everything below + // — the record, the URL, the poll — assumes the user has agreed to open + // this page, and for the two cases here they have not been asked yet. + // Nothing has been minted at this point: the flow is started but no + // approval can happen until somebody visits the login URL, so declining + // costs a request and nothing else. + val insecure = !flow.loginUrl.isHttps + if (flow.hostMismatch != null || insecure) { + unconfirmedFlow = flow + updateState { + it.copy( + step = AddAccountStep.ConfirmBrowser( + hostMismatch = flow.hostMismatch, + insecure = insecure, + ), + ) + } + return + } + launchBrowser(flow) + } + + /** The warning is read and the page is wanted anyway. */ + fun onBrowserConfirmed() { + val flow = unconfirmedFlow ?: return + unconfirmedFlow = null + viewModelScope.launch { launchBrowser(flow) } + } + + /** + * Hands [flow] to the browser and starts waiting for it. + * + * ⚠️ The record is written **here**, not where the flow was started, which + * is what [NextcloudLoginFlow.Flow]'s doc asks for: persist before launching + * the browser. Writing it at the confirmation step instead would leave a + * token behind for a page the user declined to open — a reclaim that can + * only ever poll a flow nobody will approve. + * + * ⚠️ Guarded: a DataStore write can throw, and this runs in viewModelScope, + * where an escaping exception takes the app down. A flow we failed to write + * down is a reclaim we will not get, not a reason to lose the sign-in in + * front of the user. + */ + private suspend fun launchBrowser(flow: NextcloudLoginFlow.Flow) { runCatching { pendingFlow.remember(flow) } - _state.update { + updateState { it.copy( step = AddAccountStep.WaitingForBrowser(hostMismatch = flow.hostMismatch), openInBrowser = flow.loginUrl, @@ -474,7 +647,7 @@ class AddAccountViewModel @Inject constructor( // 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) } + updateState { it.copy(originMismatch = mismatch) } } working(AddAccountMessage.Progress.ReadingLists) val outcome = gateway.discover( @@ -554,13 +727,16 @@ class AddAccountViewModel @Inject constructor( // The one exit that never reaches onStartOver: if approval already // landed, that password is about to be replaced by a typed one. discardMintedPassword() + // Also the way out of the confirmation, where the answer to "open this + // page?" is "no, I'll type a password instead". + unconfirmedFlow = null pollJob?.cancel() - _state.update { + updateState { it.copy(step = AddAccountStep.EnterCredentials(error = null), openInBrowser = null) } } - fun onBrowserLaunched() = _state.update { it.copy(openInBrowser = null) } + fun onBrowserLaunched() = updateState { it.copy(openInBrowser = null) } /** * Nothing could open the URL, so there is no approval coming. @@ -578,20 +754,20 @@ class AddAccountViewModel @Inject constructor( } fun onListToggled(url: HttpUrl) { - val step = _state.value.step as? AddAccountStep.ChooseLists ?: return + val step = current.step as? AddAccountStep.ChooseLists ?: return val selected = if (url in step.selected) step.selected - url else step.selected + url - _state.update { it.copy(step = step.copy(selected = selected)) } + updateState { it.copy(step = step.copy(selected = selected)) } } fun onSave() { - val step = _state.value.step as? AddAccountStep.ChooseLists ?: return + val step = current.step as? AddAccountStep.ChooseLists ?: return val discovered = found ?: return val chosen = step.collections.filter { it.url in step.selected }.toSet() if (username.isBlank() || appPassword.isEmpty()) { // A server that needed no credentials at all would otherwise be saved // with an empty username and password, and fail every later sync. - _state.update { it.copy(step = AddAccountStep.EnterCredentials()) } + updateState { it.copy(step = AddAccountStep.EnterCredentials()) } return } @@ -626,7 +802,7 @@ class AddAccountViewModel @Inject constructor( // Cleared before the state update: an exception there must not // leave a saved account's own credential queued for revoking. if (minted?.password == appPassword) minted = null else discardMintedPassword() - _state.update { it.copy(step = summaryOf(discovered, chosen)) } + updateState { it.copy(step = summaryOf(discovered, chosen)) } } AccountRepository.Outcome.AlreadyExists -> @@ -644,7 +820,7 @@ class AddAccountViewModel @Inject constructor( } /** The receipt is read; hand back to whoever hosts the flow. */ - fun onSummaryDone() = _state.update { it.copy(step = AddAccountStep.Done) } + fun onSummaryDone() = updateState { it.copy(step = AddAccountStep.Done) } /** * Step back inside the flow, or report that there is nowhere left to go. @@ -659,16 +835,16 @@ class AddAccountViewModel @Inject constructor( * a minted password behind and a typed address that could carry it to a * different server — the same leak `onAddressSubmitted` guards. */ - fun onBackWithin(): Boolean = when (_state.value.step) { + fun onBackWithin(): Boolean = when (current.step) { // Back into the errand when there was one, so the steps can be re-read // without starting the flow again — that is the screen people return to. is AddAccountStep.EnterAddress -> { - _state.update { it.copy(step = providerOrSetupStep(), fatal = null) } + updateState { it.copy(step = providerOrSetupStep(), fatal = null) } true } is AddAccountStep.PrepareAccess -> { - _state.update { + updateState { it.copy(step = AddAccountStep.ChooseProvider(choice), fatal = null) } true @@ -680,12 +856,23 @@ class AddAccountViewModel @Inject constructor( appPassword = "" found = null hostsNeedingAuth = emptyList() - _state.update { + updateState { it.copy(step = addressStep(typedInput), fatal = null, originMismatch = null) } true } + // ⚠️ Steps back, unlike the browser step it precedes. The note it + // carries blames an address, and "that address is wrong" is the most + // likely reading of it — so the way out has to reach the field that + // holds it. Nothing has been approved, minted or written down yet, so + // this costs the flow the server is holding and nothing else. + is AddAccountStep.ConfirmBrowser -> { + unconfirmedFlow = null + updateState { it.copy(step = addressStep(typedInput), fatal = null) } + true + } + else -> false } @@ -712,6 +899,7 @@ class AddAccountViewModel @Inject constructor( */ fun onStartOver() { discardMintedPassword() + unconfirmedFlow = null choice = null pollJob?.cancel() pollJob = null @@ -722,7 +910,7 @@ class AddAccountViewModel @Inject constructor( username = "" appPassword = "" hostsNeedingAuth = emptyList() - _state.value = AddAccountUiState() + current = AddAccountUiState() } override fun onCleared() { @@ -750,23 +938,193 @@ class AddAccountViewModel @Inject constructor( appScope.launch { runCatching { gateway.revokeIssuedAppPassword(credentials) } } } + // -------------------------------------------------------- saved state + + /** + * Writes down where the flow is, on every state change. + * + * A collector rather than a line in each of the fifteen places that update + * the state: one of those would eventually be added without it, and the + * failure is invisible until a process dies at exactly that step. + */ + private fun record(current: AddAccountUiState) { + handle[KEY_HAS_SETUP] = current.hasSetupStep + handle[KEY_STEP] = when (current.step) { + is AddAccountStep.ChooseProvider -> STEP_PROVIDER + is AddAccountStep.PrepareAccess -> STEP_SETUP + is AddAccountStep.EnterAddress -> STEP_ADDRESS + is AddAccountStep.EnterCredentials -> STEP_CREDENTIALS + // ⚠️ Recorded as the address, which is where a restore puts them. + // Neither can be resumed into: the browser is holding a flow this + // process no longer owns, and `PendingLoginFlowStore.reclaim` is + // what deals with that on the next open. Recording them as + // themselves would only mean deciding the same thing twice. + is AddAccountStep.ConfirmBrowser, is AddAccountStep.WaitingForBrowser -> STEP_ADDRESS + is AddAccountStep.Working -> STEP_WORKING + is AddAccountStep.ChooseLists -> STEP_LISTS + is AddAccountStep.Summary -> STEP_SUMMARY + AddAccountStep.Done -> STEP_DONE + } + // ⚠️ The username as it is being *typed*, not only as submitted. The + // field is the same thing [username] holds — `onCredentialsSubmitted` + // merely copies it across — and without this a restore from a + // half-filled sign-in came back with an empty name beside a password + // field, which reads as having lost both. + (current.step as? AddAccountStep.EnterCredentials)?.let { handle[KEY_USERNAME] = it.username } + // Null clears the key, so a step's payload never outlives the step. + handle[KEY_SELECTED] = (current.step as? AddAccountStep.ChooseLists) + ?.selected + ?.mapTo(ArrayList()) { it.toString() } + (current.step as? AddAccountStep.Summary).let { summary -> + handle[KEY_SUMMARY_PROVIDER] = summary?.provider?.name + handle[KEY_SUMMARY_TITLE] = summary?.title + handle[KEY_SUMMARY_SECONDARY] = summary?.secondary + handle[KEY_SUMMARY_LISTS] = summary?.lists?.let(::ArrayList) + } + } + + /** + * Picks the flow up where a killed process left it, or does nothing. + * + * Its own writes go back through [current] and so re-record what they just + * read, which is a no-op by construction — the step it restores to is the + * step it read, and a browser step deliberately records as the address it + * restores to. + */ + private fun restore() { + val step = handle.get(KEY_STEP) ?: return + val hasSetup: Boolean = handle[KEY_HAS_SETUP] ?: false + // A password the server minted before the process died is the account's + // password now; everything downstream reads it from here. + minted?.let { + username = it.username + appPassword = it.password + serverRoot = it.origin + } + when (step) { + STEP_SETUP -> { + val picked = choice + val quirk = picked?.provider?.let(ServerQuirk::forProvider) + current = if (picked != null && quirk?.hasSetupSteps == true) { + restored(AddAccountStep.PrepareAccess(picked, quirk), hasSetup) + } else { + restored(AddAccountStep.ChooseProvider(picked), hasSetup) + } + } + + STEP_ADDRESS -> current = restored(addressStep(addressInput), hasSetup) + + // The username comes back, the password does not — it is the one the + // user typed, and a restored password field is a surprise rather + // than a convenience. + STEP_CREDENTIALS -> + current = restored(AddAccountStep.EnterCredentials(username), hasSetup) + + // Both need `found`, which is not saved. One re-read of the server + // rebuilds it, and re-ticks whatever the user had ticked. + STEP_WORKING, STEP_LISTS -> resumeDiscovery(hasSetup) + + STEP_SUMMARY -> + current = restored(restoredSummary() ?: AddAccountStep.Done, hasSetup) + + STEP_DONE -> current = restored(AddAccountStep.Done, hasSetup) + + else -> current = restored(AddAccountStep.ChooseProvider(choice), hasSetup) + } + } + + private fun restored(step: AddAccountStep, hasSetup: Boolean) = AddAccountUiState( + step = step, + hasSetupStep = hasSetup, + // Derived rather than saved: it is a pure function of the choice and the + // address, both of which are. + quirk = choice?.provider?.let(ServerQuirk::forProvider) + ?: ServerQuirk.forInput(addressInput), + ) + + /** + * Re-reads the collections for a flow that died holding a credential. + * + * ⚠️ The credential is the whole test. Without one there is nothing to ask + * the server with, so the flow goes back to the address — which is the step + * that can get one — rather than to a spinner that will never resolve. + */ + private fun resumeDiscovery(hasSetup: Boolean) { + val root = serverRoot + val target = typedInput.ifBlank { root?.toString().orEmpty() } + if (root == null || appPassword.isEmpty() || target.isBlank()) { + current = restored(addressStep(addressInput), hasSetup) + return + } + val selection = handle.get>(KEY_SELECTED) + ?.mapNotNull { it.toHttpUrlOrNull() } + ?.toSet() + current = restored( + AddAccountStep.Working(AddAccountMessage.Progress.ReadingLists), + hasSetup, + ) + viewModelScope.launch { + val credentials = CalDavGateway.Credentials(username, appPassword, root) + when (val outcome = gateway.discover(target, credentials)) { + is CalDavDiscovery.Outcome.Found -> onDiscovered(outcome, selection) + is CalDavDiscovery.Outcome.NotCalDav -> backToAddress(outcome.cause) + is CalDavDiscovery.Outcome.Failed -> backToAddress(outcome.cause) + // The credential we were holding is no longer accepted, which on + // this path is indistinguishable from never having had one. + is CalDavDiscovery.Outcome.NeedsAuthentication -> { + hostsNeedingAuth = outcome.hosts + credentialsRejected() + } + CalDavDiscovery.Outcome.Unauthenticated -> credentialsRejected() + } + } + } + + private fun restoredSummary(): AddAccountStep.Summary? { + val title = handle.get(KEY_SUMMARY_TITLE) ?: return null + return AddAccountStep.Summary( + provider = handle.get(KEY_SUMMARY_PROVIDER) + ?.let { name -> CalDavProvider.entries.firstOrNull { it.name == name } }, + title = title, + secondary = handle[KEY_SUMMARY_SECONDARY], + username = username, + lists = handle.get>(KEY_SUMMARY_LISTS).orEmpty(), + ) + } + + private fun choiceOf(token: String): ProviderChoice? = if (token == OTHER_SERVER_TOKEN) { + ProviderChoice.OtherServer + } else { + CalDavProvider.entries.firstOrNull { it.name == token }?.let(ProviderChoice::Service) + } + // ------------------------------------------------------------- internals - private fun onDiscovered(outcome: CalDavDiscovery.Outcome.Found) { + /** + * @param preselected what the user had ticked before a restore, or null on + * an ordinary run. Intersected with what the server still offers, so a + * collection that has since been unshared cannot come back ticked. + */ + private fun onDiscovered( + outcome: CalDavDiscovery.Outcome.Found, + preselected: Set? = null, + ) { found = outcome if (outcome.collections.isEmpty()) { fatal(AddAccountMessage.NoUsableLists) return } - _state.update { + val offered = outcome.collections.map { it.url }.toSet() + updateState { it.copy( step = AddAccountStep.ChooseLists( collections = outcome.collections, // Everything writable, pre-ticked: the common case is "all of // them", and a read-only share is more often noise than not. - selected = outcome.collections.filterNot { c -> c.readOnly } - .map { c -> c.url } - .toSet(), + selected = preselected?.intersect(offered) + ?: outcome.collections.filterNot { c -> c.readOnly } + .map { c -> c.url } + .toSet(), ), openInBrowser = null, ) @@ -774,7 +1132,7 @@ class AddAccountViewModel @Inject constructor( } private fun working(message: AddAccountMessage.Progress) = - _state.update { it.copy(step = AddAccountStep.Working(message), fatal = null) } + updateState { it.copy(step = AddAccountStep.Working(message), fatal = null) } /** * A dead end. The step goes back to the address underneath, so the spinner is @@ -782,7 +1140,7 @@ class AddAccountViewModel @Inject constructor( * happens to render `fatal` first, and relying on that leaves the state * lying about what it is doing. */ - private fun fatal(reason: AddAccountMessage) = _state.update { + private fun fatal(reason: AddAccountMessage) = updateState { it.copy(step = addressStep(typedInput), fatal = reason) } @@ -794,7 +1152,7 @@ class AddAccountViewModel @Inject constructor( * The screen turns the cause into a translated sentence; nothing renders the * detail. */ - private fun backToAddress(cause: CalDavDiscovery.Outcome.Cause) = _state.update { + private fun backToAddress(cause: CalDavDiscovery.Outcome.Cause) = updateState { it.copy(step = addressStep(typedInput, cause)) } @@ -833,7 +1191,7 @@ class AddAccountViewModel @Inject constructor( ) } - private fun browserFailed(reason: AddAccountMessage) = _state.update { + private fun browserFailed(reason: AddAccountMessage) = updateState { // Keeps whatever the step was carrying. The host-mismatch note is often // the *explanation* for the failure, so dropping it removes the warning // exactly when it becomes worth reading. @@ -843,8 +1201,8 @@ class AddAccountViewModel @Inject constructor( } private fun updateCredentials(transform: (AddAccountStep.EnterCredentials) -> AddAccountStep.EnterCredentials) { - val step = _state.value.step as? AddAccountStep.EnterCredentials ?: return - _state.update { it.copy(step = transform(step)) } + val step = current.step as? AddAccountStep.EnterCredentials ?: return + updateState { it.copy(step = transform(step)) } } /** The provider-specific reason a correct-looking password gets rejected. */ @@ -900,5 +1258,41 @@ class AddAccountViewModel @Inject constructor( /** The server-side lifetime is 1200s; this covers it and then stops. */ const val MAX_POLL_ATTEMPTS = 600 + + /** + * ⚠️ Tokens, not `Enum.name` or an ordinal. These strings are written + * into a bundle by one build and read back by whichever build the system + * hands the state to after an update, so they have to be stable against + * a step being renamed, added or reordered — and an unknown one has to + * be survivable, which is what `restore`'s `else` is for. + */ + const val STEP_PROVIDER = "provider" + const val STEP_SETUP = "setup" + const val STEP_ADDRESS = "address" + const val STEP_CREDENTIALS = "credentials" + const val STEP_WORKING = "working" + const val STEP_LISTS = "lists" + const val STEP_SUMMARY = "summary" + const val STEP_DONE = "done" + + /** Cannot collide with a [CalDavProvider] name, which is what it stands beside. */ + const val OTHER_SERVER_TOKEN = "other-server" + + const val KEY_STEP = "add_account.step" + const val KEY_CHOICE = "add_account.choice" + const val KEY_HAS_SETUP = "add_account.has_setup" + const val KEY_ADDRESS_INPUT = "add_account.address_input" + const val KEY_TYPED_INPUT = "add_account.typed_input" + const val KEY_SERVER_ROOT = "add_account.server_root" + const val KEY_USERNAME = "add_account.username" + const val KEY_HOSTS_NEEDING_AUTH = "add_account.hosts_needing_auth" + const val KEY_SELECTED = "add_account.selected" + const val KEY_MINTED_USERNAME = "add_account.minted_username" + const val KEY_MINTED_PASSWORD = "add_account.minted_password" + const val KEY_MINTED_ORIGIN = "add_account.minted_origin" + const val KEY_SUMMARY_PROVIDER = "add_account.summary_provider" + const val KEY_SUMMARY_TITLE = "add_account.summary_title" + const val KEY_SUMMARY_SECONDARY = "add_account.summary_secondary" + const val KEY_SUMMARY_LISTS = "add_account.summary_lists" } } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/add/SignInStep.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/add/SignInStep.kt index bc760a4..1160710 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/add/SignInStep.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/add/SignInStep.kt @@ -58,6 +58,38 @@ internal fun CredentialsStep( } } +/** + * Step 3b, first half: what is wrong with the page we are about to open. + * + * ⚠️ A screen of its own, drawn *before* the Custom Tab rather than behind it. + * The login URL is where the account password is typed, so a warning about it + * has to be readable at the moment it still means something — and the primary + * action stays the way forward, because both causes are legitimate on real + * deployments and refusing outright would make Login Flow v2 unusable behind an + * ordinary reverse proxy. + */ +@Composable +internal fun ColumnScope.ConfirmBrowserStep(step: AddAccountStep.ConfirmBrowser) { + // Title and body come from StepTitle like every other fixed step's do; what + // is left here is the cause, which is the whole reason the step exists. + step.hostMismatch?.let { mismatch -> + QuirkNote( + text = stringResource( + R.string.add_account_browser_host_mismatch, + mismatch.actual, + mismatch.expected, + ), + ) + } + + // Both can be true at once — a proxy that rewrites the host and drops TLS + // is one misconfiguration, not two — and each is worth its own sentence. + if (step.insecure) { + Spacer(Modifier.height(16.dp)) + QuirkNote(text = stringResource(R.string.add_account_browser_insecure)) + } +} + /** * Step 3b: the server is signing the user in, in a browser we do not own. * diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/lists/ListEditorSheet.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/lists/ListEditorSheet.kt index d93f4c2..f24eab3 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/lists/ListEditorSheet.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/lists/ListEditorSheet.kt @@ -17,7 +17,11 @@ import androidx.compose.foundation.selection.selectable import androidx.compose.foundation.shape.CircleShape import androidx.compose.material.icons.Icons import androidx.compose.material.icons.rounded.Check +import androidx.compose.material.icons.rounded.ChevronRight +import androidx.compose.material.icons.rounded.CloudSync import androidx.compose.material.icons.rounded.DeleteOutline +import androidx.compose.material.icons.rounded.Lock +import androidx.compose.material.icons.rounded.PhoneAndroid import androidx.compose.material3.AlertDialog import androidx.compose.material3.Button import androidx.compose.material3.Icon @@ -51,36 +55,67 @@ import de.jeanlucmakiola.agendula.ui.common.DefaultListColor import de.jeanlucmakiola.agendula.ui.common.ListColorChip import de.jeanlucmakiola.agendula.ui.common.ListPalette import de.jeanlucmakiola.floret.components.FullScreenPicker +import de.jeanlucmakiola.floret.components.GroupedRow import de.jeanlucmakiola.floret.components.GroupedSurface import de.jeanlucmakiola.floret.components.InlineTextField +import de.jeanlucmakiola.floret.components.OptionPicker import de.jeanlucmakiola.floret.components.Position import de.jeanlucmakiola.floret.components.pastelize private const val SWATCHES_PER_ROW = 6 +/** + * Where a list lives. + * + * [accountId] null is the device, which is always offered and always first: it + * needs no server, no permission and no network, so it is the choice that cannot + * fail. The rest are CalDAV accounts whose home set answered OPTIONS with a + * creation method — an account that has neither is absent rather than disabled, + * because "you cannot make a list here" is not a decision the user can act on. + */ +data class ListDestination(val accountId: Long?, val label: String) + /** * Create or edit a task list: a name field over the palette of list colours, on * the family's full-screen sheet with the commit in its title bar. * - * [initial] null is the create case. [onDelete] is null for a list the app must - * not remove — an account's collection belongs to its server — which is also why - * the destructive row only appears when it is non-null. + * [initial] null is the create case, and the only one that offers a choice of + * [destinations] — moving a list between a server and the device is a copy and a + * delete rather than an edit, and is not this sheet's job. + * + * ⚠️ [onDelete] no longer means "device-only list". An account's collection can + * be deleted now, on the server, which is what the caller wires it to; it stays + * null for a list the app must not remove — a read-only share, whose removal + * belongs to whoever owns it. */ @Composable fun ListEditorSheet( initial: TaskList?, - onSave: (name: String, color: Int) -> Unit, + onSave: (name: String, color: Int, accountId: Long?) -> Unit, onDismiss: () -> Unit, onDelete: (() -> Unit)? = null, + destinations: List = emptyList(), ) { var name by rememberSaveable(initial?.id) { mutableStateOf(initial?.name.orEmpty()) } var color by rememberSaveable(initial?.id) { mutableIntStateOf(initial?.color ?: DefaultListColor) } var confirmDelete by rememberSaveable { mutableStateOf(false) } + // Saved by id rather than by object: the destination has to survive a + // rotation, and a Long? is the only part of it that is worth keeping — the + // labels are rebuilt from the accounts either way. + var destinationId by rememberSaveable(initial?.id) { mutableStateOf(initial?.accountId) } + var choosingDestination by rememberSaveable { mutableStateOf(false) } - val valid = name.isNotBlank() + // One choice is not a choice. A device with no account that can make + // collections gets the sheet it has always had. + val offersChoice = initial == null && destinations.size > 1 + val destination = destinations.firstOrNull { it.accountId == destinationId } + ?: destinations.firstOrNull() + + val readOnly = initial?.isReadOnly == true + val valid = name.isNotBlank() && !readOnly val commit = { if (valid) { - onSave(name.trim(), color) + onSave(name.trim(), color, destinationId) onDismiss() } } @@ -105,6 +140,28 @@ fun ListEditorSheet( onImeAction = commit, ) + // ⚠️ Said before the work, not after it. A list made on a server and a + // list made on the device are different things — one syncs to every + // other client, one never leaves the phone — and finding out which you + // got by watching it fail to appear elsewhere is the wrong way round. + if (offersChoice && destination != null) { + Spacer(Modifier.height(20.dp)) + SectionLabel(stringResource(R.string.list_where)) + GroupedRow( + title = destination.label, + position = Position.Alone, + modifier = Modifier.padding(horizontal = 16.dp), + leading = { Icon(destinationIcon(destination), contentDescription = null) }, + trailing = { Icon(Icons.Rounded.ChevronRight, contentDescription = null) }, + onClick = { choosingDestination = true }, + ) + } + + if (readOnly) { + Spacer(Modifier.height(20.dp)) + ReadOnlyNote() + } + Spacer(Modifier.height(20.dp)) SectionLabel(stringResource(R.string.list_color)) ListColorGrid(selected = color, onSelect = { color = it }) @@ -116,9 +173,27 @@ fun ListEditorSheet( Spacer(Modifier.height(24.dp)) } + if (choosingDestination && destination != null) { + OptionPicker( + title = stringResource(R.string.list_where), + options = destinations, + selected = destination, + label = { it.label }, + leading = { Icon(destinationIcon(it), contentDescription = null) }, + // No dismissal here: OptionPicker calls onSelect and then onDismiss + // on the same tap, and closing it twice reads as though it did not. + onSelect = { destinationId = it.accountId }, + onDismiss = { choosingDestination = false }, + ) + } + if (confirmDelete && onDelete != null) { DeleteListDialog( listName = initial?.name.orEmpty(), + // A synced list is deleted on the server too, and that reaches every + // other client the account has. Saying so is the difference between + // a confirmation and a trap. + synced = initial?.accountId != null, onConfirm = { confirmDelete = false onDelete() @@ -129,6 +204,33 @@ fun ListEditorSheet( } } +/** The device and a server are different places, so they get different marks. */ +private fun destinationIcon(destination: ListDestination) = + if (destination.accountId == null) Icons.Rounded.PhoneAndroid else Icons.Rounded.CloudSync + +/** A share we may read and not write; the edit has nowhere to land. */ +@Composable +private fun ReadOnlyNote() { + GroupedSurface( + position = Position.Alone, + modifier = Modifier.padding(horizontal = 16.dp), + color = MaterialTheme.colorScheme.surfaceVariant, + ) { + Row( + modifier = Modifier.fillMaxWidth().heightIn(min = 64.dp).padding(20.dp), + verticalAlignment = Alignment.CenterVertically, + horizontalArrangement = Arrangement.spacedBy(16.dp), + ) { + Icon(Icons.Rounded.Lock, contentDescription = null) + Text( + text = stringResource(R.string.list_read_only), + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + } + } +} + /** The name, with the chosen colour beside it so the two read as one thing. */ @Composable internal fun ListNameField( @@ -254,11 +356,27 @@ private fun DeleteRow(onClick: () -> Unit) { } @Composable -private fun DeleteListDialog(listName: String, onConfirm: () -> Unit, onDismiss: () -> Unit) { +private fun DeleteListDialog( + listName: String, + synced: Boolean, + onConfirm: () -> Unit, + onDismiss: () -> Unit, +) { AlertDialog( onDismissRequest = onDismiss, title = { Text(stringResource(R.string.list_delete_confirm_title)) }, - text = { Text(stringResource(R.string.list_delete_confirm_message, listName)) }, + text = { + Text( + stringResource( + if (synced) { + R.string.list_delete_confirm_message_synced + } else { + R.string.list_delete_confirm_message + }, + listName, + ), + ) + }, confirmButton = { TextButton(onClick = onConfirm) { Text(stringResource(R.string.delete), color = MaterialTheme.colorScheme.error) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/lists/ListsScreen.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/lists/ListsScreen.kt index fa5862a..7ebfaa8 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/lists/ListsScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/lists/ListsScreen.kt @@ -114,6 +114,12 @@ fun ListsScreen( var query by rememberSaveable { mutableStateOf("") } var searchActive by rememberSaveable { mutableStateOf(false) } var newList by rememberSaveable { mutableStateOf(false) } + val destinations by viewModel.destinations.collectAsStateWithLifecycle() + // ⚠️ Asked when the sheet opens, not when the screen is built. The answer is + // an OPTIONS against each account's home set, and an account added since — + // or a server upgraded since — has to be able to appear without a restart. + val deviceLabel = stringResource(R.string.list_where_device) + LaunchedEffect(newList) { if (newList) viewModel.refreshDestinations(deviceLabel) } val closeSearch = { query = "" searchActive = false @@ -200,7 +206,9 @@ fun ListsScreen( ) { SnackChip( visible = writeFailure != null, - message = stringResource(R.string.list_save_failed), + message = stringResource( + listWriteFailureMessage(writeFailure ?: ListWriteFailure.SAVE), + ), ) } } @@ -211,10 +219,26 @@ fun ListsScreen( initial = null, onSave = viewModel::createList, onDismiss = { newList = false }, + destinations = destinations, ) } } +/** + * Wording for a refused list write. + * + * Shared with the task list, which offers the same edit from the other side — + * one refusal should not read differently depending on where it was asked for. + */ +internal fun listWriteFailureMessage(failure: ListWriteFailure): Int = when (failure) { + ListWriteFailure.SAVE -> R.string.list_save_failed + ListWriteFailure.DELETE -> R.string.list_delete_failed + ListWriteFailure.SERVER_REFUSED -> R.string.list_save_failed_server + ListWriteFailure.OFFLINE -> R.string.list_save_failed_offline + ListWriteFailure.READ_ONLY -> R.string.list_save_failed_read_only + ListWriteFailure.UNSUPPORTED -> R.string.list_save_failed_unsupported +} + @Composable private fun ListsContent( state: ListsUiState.Content, diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/lists/ListsViewModel.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/lists/ListsViewModel.kt index 0215035..d73f1b5 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/lists/ListsViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/lists/ListsViewModel.kt @@ -3,6 +3,7 @@ package de.jeanlucmakiola.agendula.ui.lists import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel +import de.jeanlucmakiola.agendula.data.sync.RemoteListRepository import de.jeanlucmakiola.agendula.data.tasks.TasksRepository import de.jeanlucmakiola.agendula.data.tasks.recoveringFromProviderFailure import de.jeanlucmakiola.floret.time.DayWindow @@ -25,8 +26,46 @@ import kotlin.time.Clock /** * What a list write failed at. The screens turn this into wording; the view * models stay free of resources. + * + * ⚠️ The four server-side entries are not decoration. A list that lives on a + * server can fail in ways a device-only one cannot — the server refuses, the + * network is gone, the share is read-only, the account has no MKCALENDAR — and + * every one of them has a different thing for the user to do next. Collapsing + * them into SAVE tells someone on a train that their server said no. */ -enum class ListWriteFailure { SAVE, DELETE } +enum class ListWriteFailure { + SAVE, + DELETE, + + /** The server understood and refused. Retrying changes nothing. */ + SERVER_REFUSED, + + /** The server could not be reached. Worth trying again later. */ + OFFLINE, + + /** A read-only share: the write belongs to whoever owns it. */ + READ_ONLY, + + /** This account cannot make collections at all — iCloud, Posteo, Google. */ + UNSUPPORTED, +} + +/** Maps a remote outcome onto the wording the screens already know how to show. */ +internal fun RemoteListRepository.Outcome.asFailure(local: ListWriteFailure): ListWriteFailure? = + when (this) { + RemoteListRepository.Outcome.Done -> null + is RemoteListRepository.Outcome.Refused -> ListWriteFailure.SERVER_REFUSED + RemoteListRepository.Outcome.Unreachable -> ListWriteFailure.OFFLINE + RemoteListRepository.Outcome.Unsupported -> ListWriteFailure.UNSUPPORTED + RemoteListRepository.Outcome.ReadOnly -> ListWriteFailure.READ_ONLY + // The account is gone, stopped or undecryptable, or the server said + // something this call cannot place. Nothing specific to say beyond + // "that did not save", which is what [local] is — and specifically not + // "you are offline", which neither of them is a claim about. + RemoteListRepository.Outcome.NoAccount, + RemoteListRepository.Outcome.Unexpected, + -> local + } data class ListOverview(val list: TaskList, val openCount: Int) data class AccountGroup(val accountName: String, val lists: List) @@ -54,8 +93,26 @@ private const val UPCOMING_PREVIEW = 3 @HiltViewModel class ListsViewModel @Inject constructor( private val repository: TasksRepository, + private val remoteLists: RemoteListRepository, ) : ViewModel() { + /** + * Where a new list may go: the device, then every account whose server said + * it would take one. + * + * ⚠️ Refreshed when the sheet opens rather than held. The answer comes from + * an OPTIONS, and an account added — or upgraded — since this screen was + * built has to be able to appear without a restart. + */ + private val _destinations = MutableStateFlow(emptyList()) + val destinations: StateFlow> = _destinations.asStateFlow() + + fun refreshDestinations(deviceLabel: String) = viewModelScope.launch { + val accounts = runCatching { remoteLists.creatableAccounts() }.getOrDefault(emptyList()) + _destinations.value = listOf(ListDestination(null, deviceLabel)) + + accounts.map { ListDestination(it.id, it.displayName) } + } + val state: StateFlow = combine( repository.taskLists(), @@ -130,13 +187,23 @@ class ListsViewModel @Inject constructor( fun clearWriteFailure() { _writeFailure.value = null } /** - * Create a device-only list. The lists flow picks it up on the store change; - * a refusal (External mode, a provider that says no) surfaces through + * Create a list, on the device or on a server. + * + * The lists flow picks it up on the store change; a refusal — External mode, + * a provider that says no, a server that answers 403 — surfaces through * [writeFailure] rather than vanishing, because the sheet has already closed. */ - fun createList(name: String, color: Int) = viewModelScope.launch { + fun createList(name: String, color: Int, accountId: Long?) = viewModelScope.launch { if (name.isBlank()) return@launch - runCatching { repository.createLocalList(name.trim(), color) } - .onFailure { _writeFailure.value = ListWriteFailure.SAVE } + if (accountId == null) { + runCatching { repository.createLocalList(name.trim(), color) } + .onFailure { _writeFailure.value = ListWriteFailure.SAVE } + return@launch + } + // ⚠️ Server first. A row written before the MKCALENDAR would be a list + // that exists on the phone and nowhere else, with nothing to say so. + val outcome = runCatching { remoteLists.create(accountId, name.trim(), color) } + .getOrElse { RemoteListRepository.Outcome.Unreachable } + _writeFailure.value = outcome.asFailure(ListWriteFailure.SAVE) } } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/tasklist/TaskListScreen.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/tasklist/TaskListScreen.kt index e81aad9..9a7124d 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/tasklist/TaskListScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/tasklist/TaskListScreen.kt @@ -99,6 +99,7 @@ import de.jeanlucmakiola.agendula.domain.TaskSection import de.jeanlucmakiola.agendula.domain.TaskSections import de.jeanlucmakiola.agendula.ui.common.priorityAccent import de.jeanlucmakiola.agendula.ui.lists.ListEditorSheet +import de.jeanlucmakiola.agendula.ui.lists.listWriteFailureMessage import de.jeanlucmakiola.agendula.ui.lists.ListWriteFailure import de.jeanlucmakiola.floret.components.Position import de.jeanlucmakiola.floret.components.SnackChip @@ -252,19 +253,21 @@ fun TaskListScreen( if (editingList && list != null) { ListEditorSheet( initial = list, - onSave = { name, color -> viewModel.updateList(list.id, name, color) }, + onSave = { name, color, _ -> viewModel.updateList(list.id, name, color, list.accountId) }, onDismiss = { editingList = false }, - onDelete = { viewModel.deleteList(list.id) }, + // ⚠️ Absent for a read-only share, whose removal belongs to whoever + // owns it — and which the server would refuse anyway. Present for + // the account's own collections, where it now deletes on the server + // as well as here. + onDelete = if (list.isReadOnly) { + null + } else { + { viewModel.deleteList(list.id, list.accountId) } + }, ) } } -/** Wording for a refused list write. */ -private fun listWriteFailureMessage(failure: ListWriteFailure): Int = when (failure) { - ListWriteFailure.SAVE -> R.string.list_save_failed - ListWriteFailure.DELETE -> R.string.list_delete_failed -} - @OptIn(ExperimentalMaterial3Api::class, ExperimentalFoundationApi::class) @Composable private fun TaskListBody( diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/tasklist/TaskListViewModel.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/tasklist/TaskListViewModel.kt index 3729555..d2986b3 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/tasklist/TaskListViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/tasklist/TaskListViewModel.kt @@ -4,6 +4,7 @@ import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel import de.jeanlucmakiola.agendula.data.prefs.SettingsPrefs +import de.jeanlucmakiola.agendula.data.sync.RemoteListRepository import de.jeanlucmakiola.agendula.data.tasks.TasksRepository import de.jeanlucmakiola.agendula.data.tasks.recoveringFromProviderFailure import de.jeanlucmakiola.agendula.domain.Task @@ -11,6 +12,7 @@ import de.jeanlucmakiola.agendula.domain.TaskFilter import de.jeanlucmakiola.agendula.domain.TaskForm import de.jeanlucmakiola.agendula.domain.TaskList import de.jeanlucmakiola.agendula.ui.lists.ListWriteFailure +import de.jeanlucmakiola.agendula.ui.lists.asFailure import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted @@ -55,6 +57,7 @@ sealed interface TaskListUiState { class TaskListViewModel @Inject constructor( private val repository: TasksRepository, private val settingsPrefs: SettingsPrefs, + private val remoteLists: RemoteListRepository, ) : ViewModel() { private val filter = MutableStateFlow(null) @@ -170,22 +173,48 @@ class TaskListViewModel @Inject constructor( fun clearListWriteFailure() { _listWriteFailure.value = null } - /** Rename / recolour the list this screen is showing. */ - fun updateList(listId: Long, name: String, color: Int) = viewModelScope.launch { - if (name.isBlank()) return@launch - runCatching { repository.updateList(listId, name.trim(), color) } - .onFailure { _listWriteFailure.value = ListWriteFailure.SAVE } - } + /** + * Rename / recolour the list this screen is showing. + * + * ⚠️ A synced list goes to the server **first**. Writing the row and setting + * `is_dirty` is what used to happen, and nothing ever read that flag — so a + * rename looked like it worked, never left the phone, and was silently + * reverted by whichever sync next re-read the collection's display name. + */ + fun updateList(listId: Long, name: String, color: Int, accountId: Long?) = + viewModelScope.launch { + if (name.isBlank()) return@launch + if (accountId == null) { + runCatching { repository.updateList(listId, name.trim(), color) } + .onFailure { _listWriteFailure.value = ListWriteFailure.SAVE } + return@launch + } + val outcome = runCatching { remoteLists.rename(listId, name.trim(), color) } + .getOrElse { RemoteListRepository.Outcome.Unreachable } + _listWriteFailure.value = outcome.asFailure(ListWriteFailure.SAVE) + } /** * Delete the list **and its tasks**. The screen navigates away on * [listDeleted], not on the call — leaving first would strand a refusal on a * screen that no longer exists. */ - fun deleteList(listId: Long) = viewModelScope.launch { - runCatching { repository.deleteList(listId) } - .onSuccess { _listDeleted.value = true } - .onFailure { _listWriteFailure.value = ListWriteFailure.DELETE } + fun deleteList(listId: Long, accountId: Long?) = viewModelScope.launch { + if (accountId == null) { + runCatching { repository.deleteList(listId) } + .onSuccess { _listDeleted.value = true } + .onFailure { _listWriteFailure.value = ListWriteFailure.DELETE } + return@launch + } + // ⚠️ The server first here too, and this one is destructive on both + // sides. Deleting only the row left the collection on the server with + // nothing pointing at it — the list vanished from the phone, kept + // existing for every other client, and could only be got back by + // re-adding the whole account. + val outcome = runCatching { remoteLists.delete(listId) } + .getOrElse { RemoteListRepository.Outcome.Unreachable } + val failure = outcome.asFailure(ListWriteFailure.DELETE) + if (failure == null) _listDeleted.value = true else _listWriteFailure.value = failure } /** Inline "add subtask" from an expanded list group — files it under [parent]. */ diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 82b51b4..2438f6a 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -135,6 +135,14 @@ Could not save the list. Could not delete the list. Delete list? + Where + On this device only + This list is shared with you read-only, so its name and colour are the owner\u2019s to change. + \u201c%1$s\u201d and all of its tasks will be deleted from the server, and from every other device signed in to this account. This can\u2019t be undone. + The server would not accept that change. + Couldn\u2019t reach the server. Try again when you\u2019re back online. + That list is shared with you read-only. + That account doesn\u2019t let apps create task lists. “%1$s” and all of its tasks will be deleted. This can\'t be undone. Mauve Red @@ -375,6 +383,7 @@ CalDAV account Last sync didn\u2019t finish Sync now + Removing\u2026 Sign-in didn\u2019t finish Step %1$d of %2$d Where are your tasks? @@ -443,6 +452,16 @@ Sign in again to keep syncing Sign in again %1$s \u00b7 %2$s + Sync reports + When a sync replaces one of your edits with the server\u2019s copy, or stops trying to sync a task. + %1$s + %1$s \u00b7 %2$s + What the last sync changed + Got it + Replaced by the server\u2019s newer copy + Deleted on the server while you were editing it + You deleted it here, but the server changed it afterwards + Stopped syncing \u2014 the server keeps refusing this one Syncing Syncing tasks @@ -461,6 +480,10 @@ Approve Agendula in the browser, then come back here. Your password is never sent to this app — the server issues a separate app password you can revoke at any time. Open the browser again Use a password instead + Check this before you sign in + The sign-in page is on your server, not in Agendula \u2014 so it is worth a look before you type your account password there. + Open the sign-in page + That sign-in page is unencrypted (http). Anything you type there, including your account password, travels in the clear. The server sent us to %1$s, but you typed %2$s. That usually means its overwrite.cli.url setting is wrong. 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. @@ -469,4 +492,20 @@ Shared with you Add account Pick at least one list + + An edit was replaced by the server + %1$d edits were replaced by the server + + + A task has stopped syncing + %1$d tasks have stopped syncing + + + 1 sync report to read + %1$d sync reports to read + + + and %1$d more + and %1$d more + diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSupportStoreTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSupportStoreTest.kt new file mode 100644 index 0000000..2625130 --- /dev/null +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSupportStoreTest.kt @@ -0,0 +1,68 @@ +package de.jeanlucmakiola.agendula.data.sync + +import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.PreferenceDataStoreFactory +import androidx.datastore.preferences.core.Preferences +import com.google.common.truth.Truth.assertThat +import de.jeanlucmakiola.caldav.CollectionSupport +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel +import kotlinx.coroutines.test.runTest +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.io.TempDir +import java.io.File + +/** The cache in front of OPTIONS, which exists so a picker can draw at once. */ +class CollectionSupportStoreTest { + + @TempDir lateinit var directory: File + + private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Unconfined) + + private val dataStore: DataStore by lazy { + PreferenceDataStoreFactory.create(scope = scope) { File(directory, "support.preferences_pb") } + } + + private val store by lazy { CollectionSupportStore(dataStore) } + + @AfterEach fun tearDown() = scope.cancel() + + @Test + fun `an account nobody has asked about offers nothing`() = runTest { + // Unknown reads as "no": the cost of a missing affordance is one tap to + // get it back, and the cost of the other error is a 405 at the end of a + // form the user has already filled in. + assertThat(store.get(1)).isEqualTo(CollectionSupport.NONE) + } + + @Test + fun `a refreshed answer is the one that comes back`() = runTest { + val answered = store.refresh(1) { CollectionSupport(mkCalendar = false, extendedMkCol = true) } + + assertThat(answered.extendedMkCol).isTrue() + assertThat(store.get(1)).isEqualTo(answered) + } + + @Test + fun `a server that loses the capability says so on the next ask`() = runTest { + store.refresh(1) { CollectionSupport(mkCalendar = true, extendedMkCol = false) } + store.refresh(1) { CollectionSupport.NONE } + + // The whole reason this is a cache rather than the answer: a config + // change on the server has to be able to take the affordance away. + assertThat(store.get(1).canCreate).isFalse() + } + + @Test + fun `one account's answer leaves another's alone`() = runTest { + store.refresh(1) { CollectionSupport(mkCalendar = true, extendedMkCol = false) } + store.refresh(2) { CollectionSupport(mkCalendar = false, extendedMkCol = true) } + store.forget(1) + + assertThat(store.get(1)).isEqualTo(CollectionSupport.NONE) + assertThat(store.get(2).extendedMkCol).isTrue() + } +} diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncNoticeStoreTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncNoticeStoreTest.kt new file mode 100644 index 0000000..8cb577f --- /dev/null +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncNoticeStoreTest.kt @@ -0,0 +1,190 @@ +package de.jeanlucmakiola.agendula.data.sync + +import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.PreferenceDataStoreFactory +import androidx.datastore.preferences.core.Preferences +import com.google.common.truth.Truth.assertThat +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.runTest +import org.junit.jupiter.api.AfterEach +import org.junit.jupiter.api.Test +import org.junit.jupiter.api.io.TempDir +import java.io.File +import kotlin.time.Instant + +/** + * The two halves of "tell the user what the sync did". + * + * They are stored differently on purpose — one is news, the other a standing + * condition — and getting that backwards either loses the news to the next quiet + * sync or re-announces the same broken task every four hours for ever. + */ +class SyncNoticeStoreTest { + + @TempDir lateinit var directory: File + + private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Unconfined) + + private val dataStore: DataStore by lazy { + PreferenceDataStoreFactory.create(scope = scope) { File(directory, "notices.preferences_pb") } + } + + private val store by lazy { SyncNoticeStore(dataStore) } + + @AfterEach fun tearDown() = scope.cancel() + + @Test + fun `a discarded edit is kept when a later sync finds nothing`() = runTest { + store.record(ACCOUNT, at(1), listOf(report(discarded = listOf(edit("Milk"))))) + store.record(ACCOUNT, at(2), listOf(report())) + + // ⚠️ The edit is gone from the device either way. A clean sync an hour + // later does not make that untrue, and replacing the set would erase the + // one thing worth saying before anyone saw it. + val kept = store.observeAll().first() + assertThat(kept.map { it.subject }).containsExactly("Milk") + } + + @Test + fun `a resource that starts syncing again clears its own notice`() = runTest { + store.record(ACCOUNT, at(1), listOf(report(quarantined = listOf(stuck("a.ics"))))) + store.record(ACCOUNT, at(2), listOf(report())) + + // A standing condition, not news: it is re-reported for as long as it + // holds, so the absence of a report *is* the recovery. + assertThat(store.observeAll().first()).isEmpty() + } + + @Test + fun `a resource still failing is not announced twice`() = runTest { + val first = store.record(ACCOUNT, at(1), listOf(report(quarantined = listOf(stuck("a.ics"))))) + val second = store.record(ACCOUNT, at(2), listOf(report(quarantined = listOf(stuck("a.ics"))))) + + assertThat(first).hasSize(1) + // Announcing it every four hours is how a user learns to ignore the one + // that matters. It stays on the account screen throughout. + assertThat(second).isEmpty() + assertThat(store.observeAll().first()).hasSize(1) + } + + @Test + fun `a resource below the threshold says nothing`() = runTest { + val fresh = store.record( + ACCOUNT, + at(1), + listOf(report(quarantined = listOf(stuck("a.ics", failures = 1)))), + ) + + // Still being retried. "One of your tasks has stopped syncing" would be + // untrue of a single 502 from a proxy mid-restart. + assertThat(fresh).isEmpty() + assertThat(store.observeAll().first()).isEmpty() + } + + @Test + fun `two untitled edits in one run are both reported`() = runTest { + store.record( + ACCOUNT, + at(1), + listOf(report(discarded = listOf(edit(null, "uid-a"), edit(null, "uid-b")))), + ) + + // ⚠️ These are stored as a Set. Two discards from one run share + // an account, a list, a cause and a timestamp, so without the UID in the + // key they encoded identically and one silently vanished — the + // notification counted two and the screen listed one. + assertThat(store.observeAll().first()).hasSize(2) + } + + @Test + fun `two edits with the same title are both reported`() = runTest { + store.record( + ACCOUNT, + at(1), + listOf(report(discarded = listOf(edit("Milk", "uid-a"), edit("Milk", "uid-b")))), + ) + + assertThat(store.observeAll().first()).hasSize(2) + } + + @Test + fun `a quarantined task is named by its title where one is known`() = runTest { + val href = "https://server/dav/tasks/a1f9c3e2.ics" + store.record( + ACCOUNT, + at(1), + listOf(report(quarantined = listOf(stuck("a1f9c3e2.ics")))), + titles = mapOf(href to "Renew the passport"), + ) + + // "A task has stopped syncing" over a row reading `a1f9c3e2.ics` names + // nothing the user can act on — the same objection this file makes to + // showing a UID. + assertThat(store.observeAll().first().single().subject).isEqualTo("Renew the passport") + } + + @Test + fun `a quarantined resource we never stored falls back to its filename`() = runTest { + store.record(ACCOUNT, at(1), listOf(report(quarantined = listOf(stuck("a1f9c3e2.ics"))))) + + // Nothing local to name it by, so the filename is genuinely all there is. + assertThat(store.observeAll().first().single().subject).isEqualTo("a1f9c3e2.ics") + } + + @Test + fun `titles and list names survive whatever characters they contain`() = runTest { + store.record( + ACCOUNT, + at(1), + listOf( + report( + listName = "Work | Home", + discarded = listOf(edit("Pay the bill | urgent")), + ), + ), + ) + + // A list is named by its owner and a task titled by its author, so both + // can hold the separator this store writes with. + val notice = store.observeAll().first().single() + assertThat(notice.subject).isEqualTo("Pay the bill | urgent") + assertThat(notice.listName).isEqualTo("Work | Home") + } + + @Test + fun `one account's notices leave another's alone`() = runTest { + store.record(ACCOUNT, at(1), listOf(report(discarded = listOf(edit("Milk"))))) + store.record(OTHER, at(1), listOf(report(discarded = listOf(edit("Bread"))))) + store.dismiss(ACCOUNT) + + assertThat(store.observeAll().first().map { it.subject }).containsExactly("Bread") + } + + private fun report( + listName: String = "Tasks", + discarded: List = emptyList(), + quarantined: List = emptyList(), + ) = SyncReport( + listId = 1, + listName = listName, + discardedEdits = discarded, + quarantined = quarantined, + ) + + private fun edit(title: String?, uid: String = "uid-$title") = + DiscardedEdit(uid = uid, title = title, cause = DiscardedEdit.Cause.SERVER_NEWER) + + private fun stuck(href: String, failures: Int = QuarantineStore.THRESHOLD) = + QuarantinedResource(href = "https://server/dav/tasks/$href", reason = "415", failures = failures) + + private fun at(seconds: Long) = Instant.fromEpochMilliseconds(seconds * 1_000) + + private companion object { + const val ACCOUNT = 1L + const val OTHER = 2L + } +} diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModelTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModelTest.kt index a7dfd2c..1159887 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModelTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModelTest.kt @@ -1,5 +1,6 @@ package de.jeanlucmakiola.agendula.ui.accounts.add +import androidx.lifecycle.SavedStateHandle import com.google.common.truth.Truth.assertThat import de.jeanlucmakiola.agendula.data.sync.AccountCreator import de.jeanlucmakiola.agendula.data.sync.AccountRepository @@ -54,7 +55,12 @@ class AddAccountViewModelTest { */ private val appScope = CoroutineScope(dispatcher) - private fun viewModel() = AddAccountViewModel(creator, gateway, record, appScope) + /** + * A fresh wizard. [handle] is a seam for the restore tests: handing the same + * one to a second view model is exactly what process death does. + */ + private fun viewModel(handle: SavedStateHandle = SavedStateHandle()) = + AddAccountViewModel(creator, gateway, record, handle, appScope) @Nested inner class TheProviderStep { @@ -358,6 +364,116 @@ class AddAccountViewModelTest { } } + /** + * The gate in front of the Custom Tab. + * + * The login page is where the **account** password is typed, and the note + * about it used to arrive in the same state update as the URL — so it drew + * behind a browser that was already open, which is not a confirmation. + */ + @Nested + inner class ConfirmingTheLoginPage { + + @Test + fun `a host mismatch stops short of opening the browser`() = runTest(dispatcher) { + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = flow( + hostMismatch = NextcloudLoginFlow.HostMismatch( + expected = "cloud.example.com", + actual = "nextcloud", + ), + ) + + val vm = viewModel() + vm.enterAddress("https://cloud.example.com/") + vm.onAddressSubmitted() + runCurrent() + + val step = vm.state.value.step as AddAccountStep.ConfirmBrowser + assertThat(step.hostMismatch?.actual).isEqualTo("nextcloud") + // The two halves of the defect: no URL handed out, and nothing + // written down for a page nobody has agreed to open. + assertThat(vm.state.value.openInBrowser).isNull() + assertThat(record.remembered).isNull() + } + + @Test + fun `a cleartext login page is confirmed too`() = runTest(dispatcher) { + // requireSecureOrigin cannot fire here: the base was typed as http, + // so a cleartext login URL is consistent rather than a downgrade — + // and it is still the page the account password is typed into. + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = flow(loginUrl = "http://cloud.example.com/login/flow") + + val vm = viewModel() + vm.enterAddress("http://cloud.example.com/") + vm.onAddressSubmitted() + runCurrent() + + assertThat((vm.state.value.step as AddAccountStep.ConfirmBrowser).insecure).isTrue() + assertThat(vm.state.value.openInBrowser).isNull() + } + + @Test + fun `confirming opens the page and starts waiting for it`() = runTest(dispatcher) { + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = flow( + hostMismatch = NextcloudLoginFlow.HostMismatch("cloud.example.com", "nextcloud"), + ) + + val vm = viewModel() + vm.enterAddress("https://cloud.example.com/") + vm.onAddressSubmitted() + runCurrent() + vm.onBrowserConfirmed() + runCurrent() + + assertThat(vm.state.value.step).isInstanceOf(AddAccountStep.WaitingForBrowser::class.java) + assertThat(vm.state.value.openInBrowser).isNotNull() + // Persisted here rather than at the start, which is what the flow's + // own doc asks for: before the browser, not before the question. + assertThat(record.remembered).isEqualTo(gateway.loginFlow) + } + + @Test + fun `declining goes back to the address that produced the note`() = + runTest(dispatcher) { + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = flow( + hostMismatch = NextcloudLoginFlow.HostMismatch("cloud.example.com", "nextcloud"), + ) + + val vm = viewModel() + vm.enterAddress("https://cloud.example.com/") + vm.onAddressSubmitted() + runCurrent() + + // The note blames an address, so the way out reaches the field + // that holds it. Nothing was minted or written down, so there is + // nothing to hand back. + assertThat(vm.onBackWithin()).isTrue() + val step = vm.state.value.step as AddAccountStep.EnterAddress + assertThat(step.input).isEqualTo("https://cloud.example.com/") + assertThat(record.remembered).isNull() + } + + @Test + fun `a clean flow opens without asking`() = runTest(dispatcher) { + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = flow() + + val vm = viewModel() + vm.enterAddress("https://cloud.example.com/") + vm.onAddressSubmitted() + runCurrent() + + // Nothing to confirm is not a screen: the common case must not grow + // a step for the sake of the uncommon one. + assertThat(vm.state.value.step).isInstanceOf(AddAccountStep.WaitingForBrowser::class.java) + assertThat(vm.state.value.openInBrowser).isNotNull() + } + } + @Nested inner class SurvivingTheProcess { @@ -430,6 +546,128 @@ class AddAccountViewModelTest { } } + /** + * What a killed process leaves behind. + * + * The browser is a separate task, so ours is a background process while the + * user approves and the system is entitled to kill it. A second view model + * over the same [SavedStateHandle] is exactly that. + */ + @Nested + inner class RestoringAfterProcessDeath { + + @Test + fun `a minted password survives and the lists are re-read`() = runTest(dispatcher) { + val handle = SavedStateHandle() + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = flow() + gateway.pollResults += NextcloudLoginFlow.PollResult.Approved( + NextcloudLoginFlow.Credentials( + server = "https://cloud.example.com/".toHttpUrl(), + loginName = "me", + appPassword = "app-pw", + ), + ) + gateway.discoveryOutcomes += found(collection("Tasks"), collection("Shopping")) + + val first = viewModel(handle) + first.enterAddress("https://cloud.example.com/") + first.onAddressSubmitted() + advanceUntilIdle() + (first.state.value.step as AddAccountStep.ChooseLists).let { step -> + first.onListToggled(step.collections.last().url) + } + + // The process dies here — after approval, holding a one-shot app + // password the server will never issue again. + gateway.discoveryOutcomes += found(collection("Tasks"), collection("Shopping")) + val second = viewModel(handle) + advanceUntilIdle() + + val step = second.state.value.step as AddAccountStep.ChooseLists + // Re-read rather than restored: `found` is not saved, so the server + // answers for it again — with the credential that survived. + assertThat(gateway.discoveries.last().credentials?.password).isEqualTo("app-pw") + // And what the user had ticked is still ticked. + assertThat(step.selected.map { it.toString() }) + .containsExactly("https://cloud.example.com/dav/Tasks/") + } + + @Test + fun `a typed password is not written down`() = runTest(dispatcher) { + val handle = SavedStateHandle() + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.Unauthenticated + gateway.loginFlow = null + + val first = viewModel(handle) + first.enterAddress("https://cloud.example.com/") + first.onAddressSubmitted() + advanceUntilIdle() + first.onUsernameChanged("me") + first.onPasswordChanged("account-password") + + val second = viewModel(handle) + advanceUntilIdle() + + // ⚠️ The username comes back, the password does not. It is the + // user's own account password on this route, retyping it costs one + // field, and saved state is written out by the system. + val step = second.state.value.step as AddAccountStep.EnterCredentials + assertThat(step.username).isEqualTo("me") + assertThat(step.password).isEmpty() + } + + @Test + fun `the address that was typed comes back with it`() = runTest(dispatcher) { + val handle = SavedStateHandle() + val first = viewModel(handle) + first.enterAddress("https://cloud.example.com/") + + val step = viewModel(handle).state.value.step as AddAccountStep.EnterAddress + assertThat(step.input).isEqualTo("https://cloud.example.com/") + assertThat(step.choice).isEqualTo(ProviderChoice.OtherServer) + } + + @Test + fun `a service picked by name comes back as that service`() = runTest(dispatcher) { + val handle = SavedStateHandle() + viewModel(handle).onProviderChosen(ProviderChoice.Service(CalDavProvider.FASTMAIL)) + + // The errand screen is a step of its own, and it is the one the + // choice lands on — so restoring the choice has to restore that too. + val step = viewModel(handle).state.value.step as AddAccountStep.PrepareAccess + assertThat(step.choice).isEqualTo(ProviderChoice.Service(CalDavProvider.FASTMAIL)) + } + + @Test + fun `dying at the browser step lands back on the address`() = runTest(dispatcher) { + val handle = SavedStateHandle() + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = flow() + + val first = viewModel(handle) + first.enterAddress("https://cloud.example.com/") + first.onAddressSubmitted() + runCurrent() + + // Nothing to resume: the flow is out at a browser this process no + // longer owns, and the persisted record is what deals with it. + val step = viewModel(handle).state.value.step as AddAccountStep.EnterAddress + assertThat(step.input).isEqualTo("https://cloud.example.com/") + } + + @Test + fun `starting over leaves nothing to restore`() = runTest(dispatcher) { + val handle = SavedStateHandle() + val first = viewModel(handle) + first.enterAddress("https://cloud.example.com/") + first.onStartOver() + + val step = viewModel(handle).state.value.step as AddAccountStep.ChooseProvider + assertThat(step.choice).isNull() + } + } + @Nested inner class AbandoningTheBrowserFlow { @@ -856,11 +1094,15 @@ class AddAccountViewModelTest { crossHostHomeSets = emptyList(), ) - private fun flow() = NextcloudLoginFlow.Flow( - loginUrl = "https://cloud.example.com/login/flow".toHttpUrl(), + private fun flow( + loginUrl: String = "https://cloud.example.com/login/flow", + hostMismatch: NextcloudLoginFlow.HostMismatch? = null, + ) = NextcloudLoginFlow.Flow( + loginUrl = loginUrl.toHttpUrl(), pollEndpoint = "https://cloud.example.com/login/v2/poll".toHttpUrl(), pollToken = "token", deadlineEpochSeconds = Long.MAX_VALUE, + hostMismatch = hostMismatch, ) private class FakeGateway : CalDavGateway { diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CollectionAdmin.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CollectionAdmin.kt new file mode 100644 index 0000000..29169e6 --- /dev/null +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CollectionAdmin.kt @@ -0,0 +1,323 @@ +package de.jeanlucmakiola.caldav + +import at.bitfire.dav4jvm.Property +import at.bitfire.dav4jvm.XmlUtils +import at.bitfire.dav4jvm.XmlUtils.insertTag +import okhttp3.HttpUrl +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.RequestBody.Companion.toRequestBody +import java.io.IOException +import java.io.StringWriter + +/** + * What a server will let us do to the collections themselves. + * + * ⚠️ Asked, never assumed. `docs/SYNC-PLAN.md` chunk 5 makes this a named + * deliverable because the alternative is an affordance that 405s: **iCloud is + * extended-MKCOL only** and answers 405 to MKCALENDAR, **Google has neither** + * (and no VTODO either), and **Posteo disables collection creation outright**. + * Offering "new list on this account" to any of them produces a failure the user + * can do nothing about, at the end of a form they have already filled in. + */ +data class CollectionSupport( + /** RFC 4791 §5.3.1. The straightforward route, and what Nextcloud wants. */ + val mkCalendar: Boolean, + /** RFC 5689: MKCOL carrying a body that sets `resourcetype`. iCloud's route. */ + val extendedMkCol: Boolean, +) { + /** Whether a "new list here" affordance should exist at all. */ + val canCreate: Boolean get() = mkCalendar || extendedMkCol + + companion object { + /** What an OPTIONS we could not read has to mean: offer nothing. */ + val NONE = CollectionSupport(mkCalendar = false, extendedMkCol = false) + } +} + +/** How a collection write ended. */ +sealed interface CollectionOutcome { + data class Created(val url: HttpUrl) : CollectionOutcome + data object Updated : CollectionOutcome + + /** + * The server understood and refused. Terminal: retrying writes nothing and + * the user has to be told. + * + * [code] is for logs and for deciding; the caller owns the wording, the same + * rule [CalDavDiscovery.Outcome.Cause] states. + */ + data class Refused(val code: Int, val reason: String) : CollectionOutcome + + /** The server never answered. Worth retrying, unlike [Refused]. */ + data class Failed(val reason: String) : CollectionOutcome + + /** Neither creation method exists here. Nothing to retry and nothing to fix. */ + data object Unsupported : CollectionOutcome +} + +/** + * Creating, renaming, recolouring and deleting task collections. + * + * A seam, like [RemoteCalendar]: deciding *what* to do to a collection — refuse + * a read-only one, pick a name that will not collide, roll a local row back when + * the server says no — is the app's business and should not need a server to + * exercise. [DavCollectionAdmin] is the only production implementation. + */ +interface CollectionAdmin { + + /** What [homeSet] supports, as an OPTIONS rather than a guess. */ + fun support(homeSet: HttpUrl): CollectionSupport + + /** + * Makes a task collection under [homeSet]. + * + * @param name the path segment to create it at. The caller owns collision + * handling, because only it knows whether a second attempt is wanted. + */ + fun create( + homeSet: HttpUrl, + name: String, + displayName: String, + color: Int?, + support: CollectionSupport, + ): CollectionOutcome + + /** PROPPATCH: a rename, a recolour, or both. */ + fun updateProperties(url: HttpUrl, displayName: String?, color: Int?): CollectionOutcome + + /** DELETE. The tasks go with it, on the server and everywhere else. */ + fun delete(url: HttpUrl): CollectionOutcome +} + +class DavCollectionAdmin( + private val httpClient: OkHttpClient, +) : CollectionAdmin { + + /** + * ⚠️ Both headers, and neither on its own is enough. + * + * `Allow` names the methods, which is where MKCALENDAR shows up; `DAV` names + * the compliance classes, which is the only place `extended-mkcol` is ever + * advertised (RFC 5689 §5.1). A server may support extended MKCOL while + * listing plain MKCOL in `Allow` — that is the normal shape — so reading + * `Allow` alone reports iCloud as unable to create anything. + */ + override fun support(homeSet: HttpUrl): CollectionSupport = try { + // No compression: some servers have broken compression for OPTIONS, + // which is why DavResource disables it for the same request. + val request = Request.Builder() + .url(homeSet) + .method("OPTIONS", null) + .header("Content-Length", "0") + .header("Accept-Encoding", "identity") + .build() + Redirects.follow(httpClient, request).use { response -> + if (!response.isSuccessful) return CollectionSupport.NONE + val allow = headerTokens(response.headers("Allow")) + val dav = headerTokens(response.headers("DAV")) + CollectionSupport( + mkCalendar = "MKCALENDAR" in allow, + extendedMkCol = "EXTENDED-MKCOL" in dav, + ) + } + } catch (_: IOException) { + // Unknown reads as "no", which costs the user an affordance they can + // reach again on the next attempt — the other way round costs them a + // 405 at the end of a form. + CollectionSupport.NONE + } + + /** + * ⚠️ MKCALENDAR first where both exist. It is the method written for this + * job, every server that has it treats it as authoritative, and extended + * MKCOL's `resourcetype` set is refused outright by some servers that + * nonetheless advertise the class. + */ + override fun create( + homeSet: HttpUrl, + name: String, + displayName: String, + color: Int?, + support: CollectionSupport, + ): CollectionOutcome { + if (!support.canCreate) return CollectionOutcome.Unsupported + // A trailing slash, always: a collection URL that lacks one is resolved + // against its *parent* by every later `resolve`, so the first resource + // written into it lands one level up. + val url = homeSet.newBuilder().addPathSegment(name).addPathSegment("").build() + val (method, body) = if (support.mkCalendar) { + "MKCALENDAR" to mkCalendarBody(displayName, color) + } else { + "MKCOL" to extendedMkColBody(displayName, color) + } + return send(method, url, body) { CollectionOutcome.Created(url) } + } + + override fun updateProperties( + url: HttpUrl, + displayName: String?, + color: Int?, + ): CollectionOutcome { + if (displayName == null && color == null) return CollectionOutcome.Updated + return send("PROPPATCH", url, propPatchBody(displayName, color)) { + // ⚠️ The 207 is not read for per-property statuses, deliberately. A + // server that stores the name and refuses the colour answers 207 + // with a 403 propstat around `calendar-color` — and there is nothing + // for the user to do about a colour their server will not keep, + // while failing the whole rename over it would be worse. The colour + // is ours locally either way. + CollectionOutcome.Updated + } + } + + override fun delete(url: HttpUrl): CollectionOutcome { + val request = Request.Builder().url(url).delete().build() + return execute(request) { response -> + // ⚠️ 404 and 410 are success, exactly as they are for a resource: + // the point of a DELETE is for the thing to be absent, and it is. + when { + response.isSuccessful || response.code == NOT_FOUND || response.code == GONE -> + CollectionOutcome.Updated + else -> CollectionOutcome.Refused(response.code, response.message) + } + } + } + + private fun send( + method: String, + url: HttpUrl, + body: String, + onSuccess: () -> CollectionOutcome, + ): CollectionOutcome { + val request = Request.Builder() + .url(url) + .method(method, body.toRequestBody(MIME_XML)) + .build() + return execute(request) { response -> + if (response.isSuccessful) onSuccess() else { + CollectionOutcome.Refused(response.code, response.message) + } + } + } + + private fun execute( + request: Request, + grade: (okhttp3.Response) -> CollectionOutcome, + ): CollectionOutcome = try { + Redirects.follow(httpClient, request).use(grade) + } catch (e: IOException) { + CollectionOutcome.Failed(e.message ?: e.toString()) + } + + /** + * ⚠️ `VTODO` and nothing else. + * + * The component set is the one property here that changes what the + * collection *is*: a calendar created without it is a + * `supported-calendar-component-set` of the server's choosing, which on + * several is events-only — and a task list that refuses tasks is the worst + * possible outcome of "new list". + */ + private fun mkCalendarBody(displayName: String, color: Int?): String = xml { serializer -> + serializer.insertTag(MKCALENDAR) { + insertTag(SET) { + insertTag(PROP) { + insertTag(DISPLAYNAME) { text(displayName) } + insertTag(COMPONENT_SET) { + insertTag(COMP) { attribute(null, "name", COMPONENT) } + } + color?.let { insertTag(CALENDAR_COLOR) { text(hexOf(it)) } } + } + } + } + } + + /** + * RFC 5689, where the resource type is set by the request rather than by the + * method — which is the whole difference, and the reason iCloud needs this + * one. + */ + private fun extendedMkColBody(displayName: String, color: Int?): String = xml { serializer -> + serializer.insertTag(MKCOL) { + insertTag(SET) { + insertTag(PROP) { + insertTag(RESOURCETYPE) { + insertTag(COLLECTION) + insertTag(CALENDAR) + } + insertTag(DISPLAYNAME) { text(displayName) } + insertTag(COMPONENT_SET) { + insertTag(COMP) { attribute(null, "name", COMPONENT) } + } + color?.let { insertTag(CALENDAR_COLOR) { text(hexOf(it)) } } + } + } + } + } + + private fun propPatchBody(displayName: String?, color: Int?): String = xml { serializer -> + serializer.insertTag(PROPERTYUPDATE) { + insertTag(SET) { + insertTag(PROP) { + displayName?.let { insertTag(DISPLAYNAME) { text(it) } } + color?.let { insertTag(CALENDAR_COLOR) { text(hexOf(it)) } } + } + } + } + } + + private fun xml(build: (org.xmlpull.v1.XmlSerializer) -> Unit): String { + val serializer = XmlUtils.newSerializer() + val writer = StringWriter() + serializer.setOutput(writer) + serializer.setPrefix("d", XmlUtils.NS_WEBDAV) + serializer.setPrefix("c", XmlUtils.NS_CALDAV) + serializer.setPrefix("i", XmlUtils.NS_APPLE_ICAL) + serializer.startDocument("UTF-8", null) + build(serializer) + serializer.endDocument() + return writer.toString() + } + + /** + * ⚠️ `#RRGGBBAA`, which is what Apple's `calendar-color` is and what + * [at.bitfire.dav4jvm.property.CalendarColor] parses back. Writing the + * packed ARGB `Int` verbatim yields "-65536" for red, which is not a colour + * in any spelling. + */ + private fun hexOf(argb: Int): String = "#%06X%02X".format(argb and 0xFFFFFF, argb ushr 24) + + /** + * A comma-separated header, split and normalised. + * + * Repeated headers *and* comma lists, because servers use both spellings for + * `DAV` and for `Allow`. + */ + private fun headerTokens(values: List): Set = values + .flatMap { it.split(',') } + .map { it.trim().uppercase() } + .filter { it.isNotEmpty() } + .toSet() + + private companion object { + const val COMPONENT = "VTODO" + const val NOT_FOUND = 404 + const val GONE = 410 + val MIME_XML = "application/xml; charset=utf-8".toMediaType() + + val MKCALENDAR = Property.Name(XmlUtils.NS_CALDAV, "mkcalendar") + val CALENDAR = Property.Name(XmlUtils.NS_CALDAV, "calendar") + val COMPONENT_SET = Property.Name(XmlUtils.NS_CALDAV, "supported-calendar-component-set") + val COMP = Property.Name(XmlUtils.NS_CALDAV, "comp") + val MKCOL = Property.Name(XmlUtils.NS_WEBDAV, "mkcol") + val SET = Property.Name(XmlUtils.NS_WEBDAV, "set") + val PROP = Property.Name(XmlUtils.NS_WEBDAV, "prop") + val PROPERTYUPDATE = Property.Name(XmlUtils.NS_WEBDAV, "propertyupdate") + val DISPLAYNAME = Property.Name(XmlUtils.NS_WEBDAV, "displayname") + val RESOURCETYPE = Property.Name(XmlUtils.NS_WEBDAV, "resourcetype") + val COLLECTION = Property.Name(XmlUtils.NS_WEBDAV, "collection") + val CALENDAR_COLOR = Property.Name(XmlUtils.NS_APPLE_ICAL, "calendar-color") + } +} diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ResourceNames.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ResourceNames.kt index 11db21b..0994b13 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ResourceNames.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ResourceNames.kt @@ -48,4 +48,38 @@ object ResourceNames { /** A fresh name that cannot collide, for the fallback and the 412 retry. */ fun random(): String = UUID.randomUUID().toString() + EXTENSION + + /** + * A path segment for a new **collection**, derived from what the user called + * it. + * + * No extension: a collection is a directory, not a file. Derived rather than + * random for the same reason [forUid] is — someone browsing the account over + * WebDAV, or in their server's own web UI, should be able to tell which of + * these is "Shopping" — and, like [forUid], it is a convenience and never an + * identity: the href is what identifies the collection afterwards. + * + * ⚠️ Sanitised to the same class, which matters more here than it does for a + * resource: a display name is typed by a person, in their own language, and + * "Einkäufe 🛒" is an ordinary thing to call a list. What survives may be + * empty, and then a random segment is the honest answer. + */ + fun forCollection(displayName: String): String { + val cleaned = UNSAFE.replace(displayName, "-") + .trim('-', '.') + .take(MAX_BASENAME_BYTES) + .trimEnd('-', '.') + return if (cleaned.isEmpty() || cleaned.all { it == '-' }) randomCollection() else cleaned + } + + /** + * A collection segment that cannot collide. + * + * ⚠️ Also the retry, and Nextcloud is why it has to exist rather than being + * a fallback for unprintable names: its trashbin renames a deleted + * collection instead of removing it, so re-creating one under a name that + * was used before answers 403 for ever. A fresh segment is the only way + * back, and the display name is unaffected — two lists may share one. + */ + fun randomCollection(): String = UUID.randomUUID().toString() } diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CollectionAdminTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CollectionAdminTest.kt new file mode 100644 index 0000000..7bba58d --- /dev/null +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CollectionAdminTest.kt @@ -0,0 +1,181 @@ +package de.jeanlucmakiola.caldav + +import com.google.common.truth.Truth.assertThat +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After +import org.junit.Before +import org.junit.Test + +/** + * The collection-management side, against a server rather than a mock of one. + * + * The three things worth pinning are the three that fail silently: which method + * gets used, whether the request says `VTODO`, and whether a colour goes out in + * a spelling anything can read back. + */ +class CollectionAdminTest { + + private lateinit var server: MockWebServer + private lateinit var admin: DavCollectionAdmin + + @Before fun setUp() { + server = MockWebServer().apply { start() } + admin = DavCollectionAdmin(CalDavHttp.anonymous("test")) + } + + @After fun tearDown() = server.shutdown() + + private fun homeSet() = server.url("/dav/calendars/me/") + + @Test + fun `MKCALENDAR is read out of Allow and extended MKCOL out of DAV`() { + // The normal shape for an extended-MKCOL server: plain MKCOL in Allow, + // and the class advertised only in DAV. Reading Allow alone reports + // iCloud as unable to create anything at all. + server.enqueue( + MockResponse() + .setResponseCode(200) + .setHeader("Allow", "OPTIONS, GET, PROPFIND, MKCOL") + .setHeader("DAV", "1, 2, 3, calendar-access, extended-mkcol"), + ) + + val support = admin.support(homeSet()) + + assertThat(support.mkCalendar).isFalse() + assertThat(support.extendedMkCol).isTrue() + assertThat(support.canCreate).isTrue() + } + + @Test + fun `a server offering neither offers nothing`() { + server.enqueue( + MockResponse() + .setResponseCode(200) + .setHeader("Allow", "OPTIONS, GET, PROPFIND") + .setHeader("DAV", "1, 2, calendar-access"), + ) + + // Posteo disables collection creation and Google has neither. The + // affordance has to be gone rather than answering 405 at the end of a + // form the user has already filled in. + assertThat(admin.support(homeSet()).canCreate).isFalse() + } + + @Test + fun `an OPTIONS that never answers reads as no`() { + server.shutdown() + + assertThat(admin.support(homeSet())).isEqualTo(CollectionSupport.NONE) + } + + @Test + fun `creating asks for VTODO, by MKCALENDAR where it exists`() { + server.enqueue(MockResponse().setResponseCode(201)) + + val outcome = admin.create( + homeSet = homeSet(), + name = "shopping", + displayName = "Shopping", + color = 0xFF4C6FFF.toInt(), + support = CollectionSupport(mkCalendar = true, extendedMkCol = true), + ) + + val request = server.takeRequest() + assertThat(request.method).isEqualTo("MKCALENDAR") + // ⚠️ The trailing slash is load-bearing: without it every later + // `resolve` against this URL lands in the parent collection. + assertThat(request.path).isEqualTo("/dav/calendars/me/shopping/") + val body = request.body.readUtf8() + // A calendar created without the component set gets the server's + // default, which on several is events-only — a task list that refuses + // tasks. + assertThat(body).contains("VTODO") + assertThat(body).contains("Shopping") + // #RRGGBBAA, not the packed Int, which prints as "-11702273". + assertThat(body).contains("#4C6FFFFF") + assertThat(outcome).isInstanceOf(CollectionOutcome.Created::class.java) + } + + @Test + fun `a server with only extended MKCOL gets a resourcetype instead`() { + server.enqueue(MockResponse().setResponseCode(201)) + + admin.create( + homeSet = homeSet(), + name = "shopping", + displayName = "Shopping", + color = null, + support = CollectionSupport(mkCalendar = false, extendedMkCol = true), + ) + + val request = server.takeRequest() + assertThat(request.method).isEqualTo("MKCOL") + // The whole difference between the two: here the request says what the + // collection is, rather than the method. + assertThat(request.body.readUtf8()).contains("resourcetype") + } + + @Test + fun `a server with neither is refused before a request is made`() { + val outcome = admin.create( + homeSet = homeSet(), + name = "shopping", + displayName = "Shopping", + color = null, + support = CollectionSupport.NONE, + ) + + assertThat(outcome).isEqualTo(CollectionOutcome.Unsupported) + assertThat(server.requestCount).isEqualTo(0) + } + + @Test + fun `a refusal carries its status rather than becoming a failure`() { + // Nextcloud's trashbin renames a deleted collection instead of removing + // it, so re-creating one under a used name answers 403 for ever. The + // caller retries under a fresh segment, which it can only do if it can + // tell "refused" from "unreachable". + server.enqueue(MockResponse().setResponseCode(403)) + + val outcome = admin.create( + homeSet = homeSet(), + name = "shopping", + displayName = "Shopping", + color = null, + support = CollectionSupport(mkCalendar = true, extendedMkCol = false), + ) + + assertThat((outcome as CollectionOutcome.Refused).code).isEqualTo(403) + } + + @Test + fun `a rename and a recolour go out as one PROPPATCH`() { + server.enqueue(MockResponse().setResponseCode(207).setBody("")) + + val outcome = admin.updateProperties( + url = server.url("/dav/calendars/me/shopping/"), + displayName = "Groceries", + color = 0xFF00FF00.toInt(), + ) + + val request = server.takeRequest() + val body = request.body.readUtf8() + assertThat(request.method).isEqualTo("PROPPATCH") + // One request, not two: a rename and a recolour are one edit as far as + // the user is concerned, and half of one landing is the worse outcome. + assertThat(body).contains("Groceries") + assertThat(body).contains("#00FF00FF") + assertThat(outcome).isEqualTo(CollectionOutcome.Updated) + } + + @Test + fun `a collection that is already gone counts as deleted`() { + server.enqueue(MockResponse().setResponseCode(404)) + + // The point of a DELETE is for the thing to be absent, and it is. + // Treating "already gone" as a failure leaves a row nothing can remove. + assertThat(admin.delete(server.url("/dav/calendars/me/shopping/"))) + .isEqualTo(CollectionOutcome.Updated) + } +} diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ResourceNamesTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ResourceNamesTest.kt index e918965..e53c965 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ResourceNamesTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ResourceNamesTest.kt @@ -38,4 +38,31 @@ class ResourceNamesTest { @Test fun `random names do not repeat`() { assertThat(ResourceNames.random()).isNotEqualTo(ResourceNames.random()) } + + @Test + fun `a collection name keeps no extension`() { + // A collection is a directory, not a file. `.ics` on the end of one is + // wrong in a way that only shows up when someone looks at the server. + assertThat(ResourceNames.forCollection("Shopping")).isEqualTo("Shopping") + } + + @Test + fun `a name that does not survive sanitising gets a random segment`() { + // "Einkäufe 🛒" is an ordinary thing to call a list, and none of it is + // in the safe class. A random segment is the honest answer; the display + // name the user sees is unaffected. + val name = ResourceNames.forCollection("🛒") + + assertThat(name).isNotEmpty() + assertThat(name).doesNotContain("🛒") + assertThat(name).doesNotContain(ResourceNames.EXTENSION) + } + + @Test + fun `two random collection segments differ`() { + // The retry after Nextcloud's trashbin 403 depends on this: re-creating + // under a segment used before answers 403 for ever. + assertThat(ResourceNames.randomCollection()) + .isNotEqualTo(ResourceNames.randomCollection()) + } }