diff --git a/.gitignore b/.gitignore index 632e80e..bd839d4 100644 --- a/.gitignore +++ b/.gitignore @@ -69,3 +69,7 @@ Thumbs.db # Local agent notes: machine-specific build setup and on-device rules, not # anything the project itself depends on. /CLAUDE.md + +# Scratch backlog. Says so in its own header — dumped items get turned into +# real work, not committed as a list. +/req_changes.md 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 b00b0fc..ca99103 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 @@ -7,6 +7,7 @@ import de.jeanlucmakiola.agendula.data.tasks.room.TasksDatabase import de.jeanlucmakiola.caldav.CalDavDiscovery import de.jeanlucmakiola.caldav.TaskCollection import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeoutOrNull import okhttp3.HttpUrl.Companion.toHttpUrlOrNull @@ -62,6 +63,9 @@ class AccountRepository @Inject constructor( suspend fun all(): List = withContext(io) { database.accounts().all() } + /** The accounts, observed, so a sync landing updates a screen that is open. */ + fun observeAll(): Flow> = database.accounts().observeAll() + /** * Creates an account and the task lists the user chose. * diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountStateStore.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountStateStore.kt index f946ffb..f45ca51 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountStateStore.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/AccountStateStore.kt @@ -5,7 +5,9 @@ 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 javax.inject.Inject import javax.inject.Singleton @@ -28,8 +30,12 @@ class AccountStateStore @Inject constructor( @SyncStateDataStore private val dataStore: DataStore, ) { - suspend fun needingSignIn(): Set = - dataStore.data.first()[KEY].orEmpty().mapNotNull { it.toLongOrNull() }.toSet() + suspend fun needingSignIn(): Set = observeNeedingSignIn().first() + + /** Observed, so a 401 during a background sync reaches an open screen. */ + fun observeNeedingSignIn(): Flow> = dataStore.data.map { prefs -> + prefs[KEY].orEmpty().mapNotNull { it.toLongOrNull() }.toSet() + } suspend fun needsSignIn(accountId: Long): Boolean = accountId in needingSignIn() diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/AccountDao.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/AccountDao.kt index e21f14a..a734b9b 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/AccountDao.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/AccountDao.kt @@ -4,15 +4,27 @@ import androidx.room.Dao import androidx.room.Insert import androidx.room.Query import androidx.room.Update +import kotlinx.coroutines.flow.Flow import kotlin.time.Instant -/** Reads and writes over `accounts`. Unused until sync lands. */ +/** Reads and writes over `accounts`. */ @Dao interface AccountDao { @Query("SELECT * FROM accounts ORDER BY display_name") fun all(): List + /** + * The same rows, observed. + * + * ⚠️ A one-shot read cannot show a sync: the worker writes `last_sync_at` + * from a background thread minutes after the button was pressed, so a screen + * holding a snapshot goes on saying "never synced" until the user leaves and + * comes back. Room's invalidation tracker is what closes that gap. + */ + @Query("SELECT * FROM accounts ORDER BY display_name") + fun observeAll(): Flow> + @Query("SELECT * FROM accounts WHERE id = :accountId") fun account(accountId: Long): AccountEntity? 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 new file mode 100644 index 0000000..21d408e --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountDetailScreen.kt @@ -0,0 +1,211 @@ +package de.jeanlucmakiola.agendula.ui.accounts + +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Spacer +import androidx.compose.foundation.layout.fillMaxWidth +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.CloudSync +import androidx.compose.material.icons.rounded.DeleteOutline +import androidx.compose.material.icons.rounded.History +import androidx.compose.material.icons.rounded.Inventory2 +import androidx.compose.material3.Icon +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Text +import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.text.AnnotatedString +import androidx.compose.ui.text.SpanStyle +import androidx.compose.ui.text.buildAnnotatedString +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.floret.components.CollapsingScaffold +import de.jeanlucmakiola.floret.components.FullScreenPicker +import de.jeanlucmakiola.floret.components.GroupedListInset +import de.jeanlucmakiola.floret.components.GroupedRow +import de.jeanlucmakiola.floret.components.Position +import de.jeanlucmakiola.floret.components.positionOf + +/** + * One account: who it is, how its last sync went, and what can be done to it. + * + * Tapping a row in the list opens this rather than a destructive prompt — a list + * row leads somewhere, it does not fire an irreversible action — so removing an + * account is a button on the account's own screen. + */ +@Composable +internal fun AccountDetailScreen( + accountId: Long, + onBack: () -> Unit, + onRemoved: () -> Unit, + onSignInAgain: () -> Unit, + viewModel: AccountsViewModel, +) { + val accounts by viewModel.accounts.collectAsStateWithLifecycle() + val row = accounts?.firstOrNull { it.account.id == accountId } + + // The row is gone the instant it is removed, while this screen is still + // sliding away. Keeping the last one it had stops that exit animating an + // empty screen. + var lastKnown by remember(accountId) { mutableStateOf(row) } + LaunchedEffect(row) { if (row != null) lastKnown = row } + val shown = row ?: lastKnown ?: return + + val account = shown.account + val identity = account.identity() + var removing by remember { mutableStateOf(false) } + + CollapsingScaffold(title = identity.title, onBack = onBack) { + AccountHero(identity) + Spacer(Modifier.height(24.dp)) + + val actions = buildList<@Composable (Position) -> Unit> { + add { position -> + GroupedRow( + title = stringResource(R.string.accounts_last_sync), + summary = syncState(shown), + position = position, + leading = { Icon(Icons.Rounded.History, contentDescription = null) }, + ) + } + if (shown.needsSignIn) { + add { position -> + GroupedRow( + title = stringResource(R.string.accounts_sign_in_again), + position = position, + leading = { Icon(Icons.AutoMirrored.Rounded.Login, contentDescription = null) }, + onClick = onSignInAgain, + ) + } + } else { + add { position -> + GroupedRow( + title = stringResource(R.string.accounts_sync_now), + position = position, + leading = { Icon(Icons.Rounded.CloudSync, contentDescription = null) }, + onClick = { viewModel.syncNow(account) }, + ) + } + } + } + actions.forEachIndexed { index, action -> action(positionOf(index, actions.size)) } + + Spacer(Modifier.height(24.dp)) + + // Its own group, away from the things that are safe to press twice. + GroupedRow( + title = errorTitle(stringResource(R.string.accounts_remove)), + position = Position.Alone, + leading = { + Icon( + Icons.Rounded.DeleteOutline, + contentDescription = null, + tint = MaterialTheme.colorScheme.error, + ) + }, + onClick = { removing = true }, + ) + Spacer(Modifier.height(24.dp)) + } + + if (removing) { + RemoveAccountPicker( + identity = identity, + onDismiss = { removing = false }, + onRemove = { deleteLocalData -> + viewModel.remove(account, deleteLocalData) + removing = false + onRemoved() + }, + ) + } +} + +/** The account's logo at full size, over the two things the title bar left out. */ +@Composable +private fun AccountHero(identity: AccountIdentity) { + Column( + modifier = Modifier + .fillMaxWidth() + .padding(horizontal = GroupedListInset), + horizontalAlignment = Alignment.CenterHorizontally, + ) { + ProviderLogo(identity.provider, size = 72.dp) + Spacer(Modifier.height(12.dp)) + identity.user?.let { + Text(it, style = MaterialTheme.typography.titleMedium) + } + Text( + identity.secondary ?: stringResource(R.string.accounts_generic_provider), + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + } +} + +/** + * Removing an account is two different things — the lists stay behind as + * device-only lists, or they go with it — so it is a browse-style choice, and + * those are full-screen. Each row says what it does and does it; leaving without + * choosing is back. + */ +@Composable +private fun RemoveAccountPicker( + identity: AccountIdentity, + onDismiss: () -> Unit, + onRemove: (deleteLocalData: Boolean) -> Unit, +) { + FullScreenPicker( + title = stringResource(R.string.accounts_remove), + onDismiss = onDismiss, + predictiveBack = true, + ) { + Text( + stringResource(R.string.accounts_remove_body, identity.title), + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + modifier = Modifier.padding(horizontal = GroupedListInset), + ) + Spacer(Modifier.height(16.dp)) + GroupedRow( + title = stringResource(R.string.accounts_remove_keep), + summary = stringResource(R.string.accounts_remove_keep_body), + position = Position.Top, + leading = { Icon(Icons.Rounded.Inventory2, contentDescription = null) }, + onClick = { onRemove(false) }, + ) + GroupedRow( + title = errorTitle(stringResource(R.string.accounts_remove_wipe)), + summary = AnnotatedString(stringResource(R.string.accounts_remove_wipe_body)), + position = Position.Bottom, + leading = { + Icon( + Icons.Rounded.DeleteOutline, + contentDescription = null, + tint = MaterialTheme.colorScheme.error, + ) + }, + onClick = { onRemove(true) }, + ) + } +} + +/** A destructive row's title, in the error colour the row itself cannot take. */ +@Composable +private fun errorTitle(text: String): AnnotatedString { + val error = MaterialTheme.colorScheme.error + return remember(text, error) { + buildAnnotatedString { withStyle(SpanStyle(color = error)) { append(text) } } + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountIdentity.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountIdentity.kt new file mode 100644 index 0000000..7f834de --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountIdentity.kt @@ -0,0 +1,135 @@ +package de.jeanlucmakiola.agendula.ui.accounts + +import androidx.compose.foundation.background +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.size +import androidx.compose.foundation.shape.CircleShape +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.rounded.AlternateEmail +import androidx.compose.material.icons.rounded.Cloud +import androidx.compose.material.icons.rounded.CloudSync +import androidx.compose.material.icons.rounded.Dns +import androidx.compose.material3.Icon +import androidx.compose.material3.MaterialTheme +import androidx.compose.runtime.Composable +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.draw.clip +import androidx.compose.ui.graphics.Color +import androidx.compose.ui.res.painterResource +import androidx.compose.ui.unit.Dp +import androidx.compose.ui.unit.dp +import de.jeanlucmakiola.agendula.R +import de.jeanlucmakiola.agendula.data.tasks.room.AccountEntity +import de.jeanlucmakiola.caldav.CalDavProvider +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull + +/** + * What an account is called on screen, and by what mark. + * + * `display_name` is `user@host` — the right identity for the system account and + * for the sync trigger that keys off it, and far too long for a row title. Both + * halves are stored separately anyway, so the screen shows the one that + * identifies the account and demotes the other to the supporting line. + */ +internal data class AccountIdentity( + val provider: CalDavProvider?, + /** The name: a hosted service's brand, or the host of a server the user runs. */ + val title: String, + /** The half [title] left out — the host under a brand, the software under a host. */ + val secondary: String?, + /** Who, on that server. */ + val user: String?, +) + +internal fun AccountEntity.identity(): AccountIdentity { + val url = principalUrl?.toHttpUrlOrNull() + val provider = url?.let { CalDavProvider.forPrincipal(it) } + val host = url?.host?.removePrefix("www.") + val hosted = provider?.hosted == true + return AccountIdentity( + provider = provider, + // The fallback is the stored name: an account with no principal URL + // never got past discovery, so there is nothing better to call it. + title = if (hosted) provider.label else host ?: displayName, + secondary = if (hosted) host else provider?.label, + user = username?.takeIf { it.isNotBlank() }, + ) +} + +/** + * The provider's logo: its mark, in white, on a disc of its own brand colour — + * the 40dp leading avatar Calendula gives a synced calendar, in colour, so a + * list of accounts is scannable by mark rather than by reading hostnames. + * + * White-on-brand rather than a brand-tinted glyph on a neutral chip, because it + * is the shape the marks are actually drawn in and it is the one treatment that + * needs no second colour for dark mode. Providers with no brand colour of their + * own take the app's [MaterialTheme] accent and say what they are — a mail + * service, a server, a cloud. + */ +@Composable +internal fun ProviderLogo(provider: CalDavProvider?, size: Dp = 40.dp) { + val accent = provider?.accent + Box( + modifier = Modifier + .size(size) + .clip(CircleShape) + .background(accent ?: MaterialTheme.colorScheme.primary), + contentAlignment = Alignment.Center, + ) { + val tint = if (accent != null) Color.White else MaterialTheme.colorScheme.onPrimary + val glyph = Modifier.size(size * GLYPH_FRACTION) + if (provider == CalDavProvider.NEXTCLOUD) { + Icon( + painter = painterResource(R.drawable.ic_provider_nextcloud), + contentDescription = provider.label, + tint = tint, + modifier = glyph, + ) + } else { + Icon( + imageVector = when (provider) { + CalDavProvider.BAIKAL, + CalDavProvider.DAVICAL, + CalDavProvider.SOGO, + -> Icons.Rounded.Dns + + CalDavProvider.ICLOUD -> Icons.Rounded.Cloud + + null -> Icons.Rounded.CloudSync + + else -> Icons.Rounded.AlternateEmail + }, + contentDescription = provider?.label, + tint = tint, + modifier = glyph, + ) + } + } +} + +/** + * The provider's own brand colour, where it publishes one recognisable enough to + * be worth carrying. Null means "we would be inventing it" — Baïkal, DAViCal and + * SOGo have no colour anyone would recognise, so they take the theme's accent + * instead of a made-up one. + * + * Each is dark enough to carry a white mark, which is the whole treatment: no + * dark-mode variant is needed because neither colour in the pair moves. + */ +private val CalDavProvider.accent: Color? + get() = when (this) { + CalDavProvider.NEXTCLOUD -> Color(0xFF0082C9) + CalDavProvider.ICLOUD -> Color(0xFF007AFF) + CalDavProvider.GOOGLE -> Color(0xFF1A73E8) + CalDavProvider.FASTMAIL -> Color(0xFF2B6CB0) + CalDavProvider.MAILBOX_ORG -> Color(0xFF0069B4) + CalDavProvider.POSTEO -> Color(0xFF5E9B23) + CalDavProvider.ZOHO -> Color(0xFFE42527) + CalDavProvider.YANDEX -> Color(0xFFE03A1B) + CalDavProvider.BAIKAL, CalDavProvider.DAVICAL, CalDavProvider.SOGO -> null + } + +/** The mark sits on the disc the way a launcher icon does — a little over half. */ +private const val GLYPH_FRACTION = 0.55f 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 5d74654..b9133b5 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 @@ -2,39 +2,26 @@ package de.jeanlucmakiola.agendula.ui.accounts import android.text.format.DateUtils import androidx.compose.foundation.layout.Spacer -import androidx.compose.foundation.layout.Row -import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.padding -import androidx.compose.foundation.layout.width -import androidx.compose.foundation.selection.toggleable 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.Login import androidx.compose.material.icons.rounded.Sync -import androidx.compose.material3.AlertDialog -import androidx.compose.material3.Checkbox import androidx.compose.material3.Icon import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text -import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue -import androidx.compose.runtime.mutableStateOf -import androidx.compose.runtime.remember -import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier -import androidx.compose.ui.Alignment import androidx.compose.ui.res.stringResource -import androidx.compose.ui.semantics.Role import androidx.compose.ui.unit.dp import androidx.lifecycle.compose.collectAsStateWithLifecycle import de.jeanlucmakiola.agendula.R -import de.jeanlucmakiola.agendula.data.tasks.room.AccountEntity -import de.jeanlucmakiola.agendula.ui.common.OnResume import de.jeanlucmakiola.floret.components.CollapsingScaffold +import de.jeanlucmakiola.floret.components.GroupedListInset import de.jeanlucmakiola.floret.components.GroupedRow import de.jeanlucmakiola.floret.components.Position import de.jeanlucmakiola.floret.components.positionOf @@ -43,15 +30,11 @@ import de.jeanlucmakiola.floret.components.positionOf @Composable internal fun AccountsScreen( onAddAccount: () -> Unit, + onOpenAccount: (Long) -> Unit, onBack: () -> Unit, viewModel: AccountsViewModel, ) { val accounts by viewModel.accounts.collectAsStateWithLifecycle() - var pendingRemoval by remember { mutableStateOf(null) } - var deleteLocalData by remember { mutableStateOf(false) } - - // An account can be removed from system Settings while we are away. - OnResume { viewModel.refresh() } CollapsingScaffold( title = stringResource(R.string.settings_section_accounts), @@ -63,50 +46,36 @@ internal fun AccountsScreen( Icon( Icons.Rounded.CloudSync, contentDescription = null, - modifier = Modifier.padding(horizontal = 16.dp), + modifier = Modifier.padding(horizontal = GroupedListInset), ) Spacer(Modifier.height(12.dp)) Text( stringResource(R.string.accounts_empty_title), style = MaterialTheme.typography.titleMedium, - modifier = Modifier.padding(horizontal = 16.dp), + modifier = Modifier.padding(horizontal = GroupedListInset), ) Spacer(Modifier.height(4.dp)) Text( stringResource(R.string.accounts_empty_body), style = MaterialTheme.typography.bodyMedium, color = MaterialTheme.colorScheme.onSurfaceVariant, - modifier = Modifier.padding(horizontal = 16.dp), + modifier = Modifier.padding(horizontal = GroupedListInset), ) Spacer(Modifier.height(24.dp)) } else { loaded.forEachIndexed { index, row -> val account = row.account + val identity = account.identity() GroupedRow( - title = account.displayName, - // Never the raw values: lastSyncError is an exception string - // and lastSyncAt renders as an ISO-8601 UTC instant, and both - // bypass strings.xml entirely. - summary = when { - // Distinct from a failed sync on purpose: this one has - // stopped retrying, and only the user can restart it. - row.needsSignIn -> stringResource(R.string.accounts_needs_sign_in) - account.lastSyncError != null -> - stringResource(R.string.accounts_sync_failed) - account.lastSyncAt != null -> DateUtils.getRelativeTimeSpanString( - account.lastSyncAt.toEpochMilliseconds(), - System.currentTimeMillis(), - DateUtils.MINUTE_IN_MILLIS, - ).toString() - else -> stringResource(R.string.accounts_never_synced) - }, + title = identity.title, + summary = accountSummary(identity.user, syncState(row)), position = positionOf(index, loaded.size), - modifier = Modifier.padding(horizontal = 16.dp), + leading = { ProviderLogo(identity.provider) }, trailing = { if (row.needsSignIn) { IconButton(onClick = onAddAccount) { Icon( - Icons.Rounded.Login, + Icons.AutoMirrored.Rounded.Login, contentDescription = stringResource(R.string.accounts_sign_in_again), ) } @@ -119,7 +88,7 @@ internal fun AccountsScreen( } } }, - onClick = { pendingRemoval = account }, + onClick = { onOpenAccount(account.id) }, ) } Spacer(Modifier.height(24.dp)) @@ -128,53 +97,32 @@ internal fun AccountsScreen( GroupedRow( title = stringResource(R.string.accounts_add), position = Position.Alone, - modifier = Modifier.padding(horizontal = 16.dp), leading = { Icon(Icons.Rounded.Add, contentDescription = null) }, onClick = onAddAccount, ) } - - // A plain confirmation, which is the one thing CLAUDE.md still allows an - // AlertDialog for. The checkbox modifies the confirmation rather than turning - // it into a chooser — a full-screen picker for "are you sure" would be worse, - // and a radio list is banned outright. - pendingRemoval?.let { account -> - AlertDialog( - onDismissRequest = { pendingRemoval = null; deleteLocalData = false }, - title = { Text(stringResource(R.string.accounts_remove_confirm_title, account.displayName)) }, - text = { - Column { - Text(stringResource(R.string.accounts_remove_confirm_body)) - Spacer(Modifier.height(16.dp)) - Row( - verticalAlignment = Alignment.CenterVertically, - modifier = Modifier.toggleable( - value = deleteLocalData, - role = Role.Checkbox, - onValueChange = { deleteLocalData = it }, - ), - ) { - Checkbox(checked = deleteLocalData, onCheckedChange = null) - Spacer(Modifier.width(12.dp)) - Text( - stringResource(R.string.accounts_remove_delete_local), - style = MaterialTheme.typography.bodyMedium, - ) - } - } - }, - confirmButton = { - TextButton(onClick = { - viewModel.remove(account, deleteLocalData) - pendingRemoval = null - deleteLocalData = false - }) { Text(stringResource(R.string.accounts_remove_confirm_action)) } - }, - dismissButton = { - TextButton(onClick = { pendingRemoval = null; deleteLocalData = false }) { - Text(stringResource(android.R.string.cancel)) - } - }, - ) - } } + +/** + * Never the raw values: `lastSyncError` is an exception string and `lastSyncAt` + * renders as an ISO-8601 UTC instant, and both bypass `strings.xml` entirely. + */ +@Composable +internal fun syncState(row: AccountsViewModel.AccountRow): String = when { + // Distinct from a failed sync on purpose: this one has stopped retrying, and + // only the user can restart it. + row.needsSignIn -> stringResource(R.string.accounts_needs_sign_in) + row.account.lastSyncError != null -> stringResource(R.string.accounts_sync_failed) + row.account.lastSyncAt != null -> DateUtils.getRelativeTimeSpanString( + row.account.lastSyncAt.toEpochMilliseconds(), + System.currentTimeMillis(), + DateUtils.MINUTE_IN_MILLIS, + ).toString() + + else -> stringResource(R.string.accounts_never_synced) +} + +/** Who, then how it last went — the two things the title does not already say. */ +@Composable +private fun accountSummary(user: String?, state: String): String = + if (user == null) state else stringResource(R.string.accounts_summary, user, state) 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 df2290f..f52a44b 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 @@ -7,9 +7,10 @@ import de.jeanlucmakiola.agendula.data.sync.AccountRepository import de.jeanlucmakiola.agendula.data.sync.AccountStateStore 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.asStateFlow +import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.launch import javax.inject.Inject @@ -23,21 +24,26 @@ class AccountsViewModel @Inject constructor( /** An account row, plus the one thing the row cannot read from the entity. */ data class AccountRow(val account: AccountEntity, val needsSignIn: Boolean) - private val _accounts = MutableStateFlow?>(null) - - /** `null` until the first load, so the empty state does not flash. */ - val accounts: StateFlow?> = _accounts.asStateFlow() - - init { - refresh() - } - - fun refresh() { - viewModelScope.launch { - val stopped = accountState.needingSignIn() - _accounts.value = repository.all().map { AccountRow(it, it.id in stopped) } - } - } + /** + * `null` until the first load, so the empty state does not flash. + * + * ⚠️ Observed, not fetched. The sync that a tap on this screen starts + * finishes on a background thread some seconds later, and a snapshot taken + * when the screen opened cannot show it — the row went on saying "never + * synced" until the user left and came back. This also carries a *background* + * sync, and a 401 that stops an account, onto a screen already open. + */ + val accounts: StateFlow?> = + combine( + repository.observeAll(), + accountState.observeNeedingSignIn(), + ) { accounts, stopped -> + accounts.map { AccountRow(it, it.id in stopped) } + }.stateIn( + scope = viewModelScope, + started = SharingStarted.WhileSubscribed(STOP_TIMEOUT_MILLIS), + initialValue = null, + ) /** * The app's own sync trigger. @@ -54,9 +60,13 @@ class AccountsViewModel @Inject constructor( } fun remove(account: AccountEntity, deleteLocalData: Boolean) { - viewModelScope.launch { - repository.remove(account.id, account.displayName, deleteLocalData) - refresh() - } + // No refresh: the row disappears because the query behind `accounts` + // re-emits. + viewModelScope.launch { repository.remove(account.id, account.displayName, deleteLocalData) } + } + + private companion object { + /** Survives a configuration change without re-subscribing. */ + const val STOP_TIMEOUT_MILLIS = 5_000L } } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountScreen.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountScreen.kt index b753d5d..b5feda4 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountScreen.kt @@ -4,21 +4,29 @@ import android.content.ActivityNotFoundException import android.content.Intent import androidx.browser.customtabs.CustomTabsIntent import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.ColumnScope import androidx.compose.foundation.layout.Spacer +import androidx.compose.foundation.background +import androidx.compose.foundation.shape.CircleShape import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.size import androidx.compose.foundation.layout.padding import androidx.compose.foundation.text.KeyboardOptions import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.automirrored.rounded.ArrowBack import androidx.compose.material.icons.rounded.CloudOff +import androidx.compose.material.icons.rounded.Checklist +import androidx.compose.material.icons.rounded.CloudSync +import androidx.compose.material.icons.rounded.Lock +import androidx.compose.material.icons.rounded.OpenInBrowser import androidx.compose.material3.Button -import androidx.compose.material3.Checkbox import androidx.compose.material3.CircularProgressIndicator import androidx.compose.material3.Icon +import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme -import androidx.compose.material3.OutlinedTextField import androidx.compose.material3.Text import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable @@ -26,21 +34,31 @@ import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier +import androidx.compose.ui.draw.clip import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.res.stringResource +import androidx.compose.ui.text.style.TextAlign import androidx.compose.ui.text.input.ImeAction +import androidx.compose.ui.text.input.KeyboardCapitalization import androidx.compose.ui.text.input.KeyboardType import androidx.compose.ui.text.input.PasswordVisualTransformation +import androidx.compose.ui.text.input.VisualTransformation import androidx.compose.ui.unit.dp import androidx.core.net.toUri import androidx.lifecycle.compose.collectAsStateWithLifecycle import androidx.hilt.navigation.compose.hiltViewModel import de.jeanlucmakiola.agendula.R +import de.jeanlucmakiola.caldav.CalDavDiscovery import de.jeanlucmakiola.caldav.ServerQuirk import de.jeanlucmakiola.floret.components.CollapsingScaffold import de.jeanlucmakiola.floret.components.GroupedRow import de.jeanlucmakiola.floret.components.GroupedSurface +import de.jeanlucmakiola.floret.components.InlineTextField +import de.jeanlucmakiola.floret.components.OnboardingProgress +import de.jeanlucmakiola.floret.components.OnboardingScaffold +import de.jeanlucmakiola.floret.components.OnboardingSpace import de.jeanlucmakiola.floret.components.Position +import de.jeanlucmakiola.floret.components.SelectedCheck import de.jeanlucmakiola.floret.components.positionOf /** @@ -86,26 +104,56 @@ internal fun AddAccountScreen( viewModel.onBrowserLaunched() } - CollapsingScaffold( - title = stringResource(R.string.add_account_title), - // Backing out abandons a flow that may still be polling the server every - // two seconds for the rest of its twenty-minute window. - onBack = { - viewModel.onStartOver() - onBack() + // Backing out abandons a flow that may still be polling the server every two + // seconds for the rest of its twenty-minute window. + val abandon = { + viewModel.onStartOver() + onBack() + } + + OnboardingScaffold( + hero = { StepHero(state) }, + // Grouped rows and fields carry their own inset, so the column adds none. + contentPadding = 0.dp, + topSpacing = OnboardingSpace.md, + progress = { + val position = state.step.position + if (position != null) { + OnboardingProgress( + step = position, + total = ADD_ACCOUNT_STEPS, + label = stringResource( + R.string.add_account_step_of, + position, + ADD_ACCOUNT_STEPS, + ), + ) + } }, + navigationIcon = { + IconButton(onClick = abandon) { + Icon( + Icons.AutoMirrored.Rounded.ArrowBack, + contentDescription = stringResource(R.string.back), + ) + } + }, + actions = { StepActions(state, viewModel) }, ) { val fatal = state.fatal if (fatal != null) { - Message(icon = { Icon(Icons.Rounded.CloudOff, contentDescription = null) }, text = fatal) - Spacer(Modifier.height(16.dp)) - Button( - onClick = viewModel::onStartOver, - modifier = Modifier.padding(horizontal = 16.dp), - ) { Text(stringResource(R.string.add_account_start_over)) } - return@CollapsingScaffold + Text( + fatal, + style = MaterialTheme.typography.bodyLarge, + color = MaterialTheme.colorScheme.onSurfaceVariant, + textAlign = TextAlign.Center, + modifier = Modifier.padding(horizontal = 24.dp), + ) + return@OnboardingScaffold } + StepTitle(state.step) + when (val step = state.step) { is AddAccountStep.EnterServer -> ServerStep(step, state.quirk, viewModel) is AddAccountStep.Working -> WorkingStep(step) @@ -117,6 +165,74 @@ internal fun AddAccountScreen( } } +/** Which of the flow's visible steps this is, or null for one that has no place. */ +private val AddAccountStep.position: Int? + get() = when (this) { + is AddAccountStep.EnterServer -> 1 + is AddAccountStep.EnterCredentials, is AddAccountStep.WaitingForBrowser -> 2 + is AddAccountStep.ChooseLists -> 3 + // Working is a moment inside whichever step spawned it, and Done is gone + // before it draws — neither is a place the user can be. + is AddAccountStep.Working, AddAccountStep.Done -> null + } + +private const val ADD_ACCOUNT_STEPS = 3 + +/** The mark for the step, in the family's tonal circle. */ +@Composable +private fun StepHero(state: AddAccountUiState) { + val icon = when { + state.fatal != null -> Icons.Rounded.CloudOff + state.step is AddAccountStep.EnterCredentials -> Icons.Rounded.Lock + state.step is AddAccountStep.WaitingForBrowser -> Icons.Rounded.OpenInBrowser + state.step is AddAccountStep.ChooseLists -> Icons.Rounded.Checklist + else -> Icons.Rounded.CloudSync + } + Box( + modifier = Modifier + .size(72.dp) + .clip(CircleShape) + .background(MaterialTheme.colorScheme.secondaryContainer), + contentAlignment = Alignment.Center, + ) { + Icon( + icon, + contentDescription = null, + tint = MaterialTheme.colorScheme.onSecondaryContainer, + modifier = Modifier.size(34.dp), + ) + } +} + +/** Title and one line of explanation, centred above whatever the step asks for. */ +@Composable +private fun StepTitle(step: AddAccountStep) { + val (title, body) = when (step) { + is AddAccountStep.EnterServer -> + R.string.add_account_server_title to R.string.add_account_server_body + is AddAccountStep.EnterCredentials -> + R.string.add_account_credentials_title to R.string.add_account_credentials_body + is AddAccountStep.ChooseLists -> + R.string.add_account_lists_title to R.string.add_account_lists_body + else -> return + } + Text( + stringResource(title), + style = MaterialTheme.typography.headlineSmall, + textAlign = TextAlign.Center, + modifier = Modifier.padding(horizontal = 24.dp), + ) + Spacer(Modifier.height(8.dp)) + Text( + stringResource(body), + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + textAlign = TextAlign.Center, + modifier = Modifier.padding(horizontal = 24.dp), + ) + Spacer(Modifier.height(OnboardingSpace.lg)) +} + @Composable private fun ServerStep( step: AddAccountStep.EnterServer, @@ -124,69 +240,219 @@ private fun ServerStep( viewModel: AddAccountViewModel, ) { Column(Modifier.padding(horizontal = 16.dp), verticalArrangement = Arrangement.spacedBy(16.dp)) { - OutlinedTextField( + FieldRow( + label = stringResource(R.string.add_account_server_label), value = step.input, onValueChange = viewModel::onServerInputChanged, - label = { Text(stringResource(R.string.add_account_server_label)) }, - placeholder = { Text(stringResource(R.string.add_account_server_hint)) }, - isError = step.error != null, - supportingText = step.error?.let { { Text(it) } }, - singleLine = true, - keyboardOptions = KeyboardOptions( - keyboardType = KeyboardType.Uri, - imeAction = ImeAction.Go, - ), - modifier = Modifier.fillMaxWidth(), + placeholder = stringResource(R.string.add_account_server_hint), + error = step.error?.let { stringResource(it.message) }, + keyboardType = KeyboardType.Uri, + onImeAction = viewModel::onServerSubmitted, ) // Named before the attempt, not after a 401 the user cannot act on. quirk?.let { QuirkNote(it) } - Button( - onClick = viewModel::onServerSubmitted, - enabled = step.input.isNotBlank(), - modifier = Modifier.fillMaxWidth(), - ) { Text(stringResource(R.string.add_account_continue)) } } } @Composable private fun CredentialsStep(step: AddAccountStep.EnterCredentials, viewModel: AddAccountViewModel) { Column(Modifier.padding(horizontal = 16.dp), verticalArrangement = Arrangement.spacedBy(16.dp)) { - OutlinedTextField( - value = step.username, - onValueChange = viewModel::onUsernameChanged, - label = { Text(stringResource(R.string.add_account_username_label)) }, - singleLine = true, - modifier = Modifier.fillMaxWidth(), - ) - OutlinedTextField( - value = step.password, - onValueChange = viewModel::onPasswordChanged, - label = { Text(stringResource(R.string.add_account_password_label)) }, - visualTransformation = PasswordVisualTransformation(), - isError = step.error != null, - supportingText = { - Text(step.error ?: stringResource(R.string.add_account_password_hint)) - }, - singleLine = true, - keyboardOptions = KeyboardOptions( + // Two fields in one connected group, which is what the family does with + // a pair that is filled in together. + Column { + FieldRow( + label = stringResource(R.string.add_account_username_label), + value = step.username, + onValueChange = viewModel::onUsernameChanged, + position = Position.Top, + ) + FieldRow( + label = stringResource(R.string.add_account_password_label), + value = step.password, + onValueChange = viewModel::onPasswordChanged, + position = Position.Bottom, + error = step.error, + hint = stringResource(R.string.add_account_password_hint), keyboardType = KeyboardType.Password, - imeAction = ImeAction.Go, - ), - modifier = Modifier.fillMaxWidth(), + onImeAction = viewModel::onCredentialsSubmitted, + ) + } + } +} + +/** + * A discovery failure, in words rather than a status code. + * + * ⚠️ The server's own message never reaches the screen. The most common failure + * here is a **405** — what an ordinary web server answers to `PROPFIND`, and so + * what someone typing their *website* instead of their CalDAV address gets — and + * "HTTP 405 Method Not Allowed" tells them nothing they can act on. It is also + * untranslatable, and frequently not in a language they read. + */ +private val CalDavDiscovery.Outcome.Cause.message: Int + get() = when (this) { + CalDavDiscovery.Outcome.Cause.NOT_AN_ADDRESS -> R.string.add_account_error_not_an_address + CalDavDiscovery.Outcome.Cause.NOT_A_DAV_SERVER -> R.string.add_account_error_not_dav + CalDavDiscovery.Outcome.Cause.NO_CALENDAR_SUPPORT -> R.string.add_account_error_no_calendar + CalDavDiscovery.Outcome.Cause.UNREACHABLE -> R.string.add_account_error_unreachable + CalDavDiscovery.Outcome.Cause.INSECURE -> R.string.add_account_error_insecure + CalDavDiscovery.Outcome.Cause.NO_CALENDARS -> R.string.add_account_error_no_calendars + CalDavDiscovery.Outcome.Cause.SERVER_ERROR -> R.string.add_account_error_server + } + +/** + * The family's text input: a tonal grouped surface with a borderless field in + * it, never Material's outlined box. + * + * The label sits above the value rather than floating into a notch, because a + * `GroupedSurface` has no outline for a notch to interrupt. + */ +@Composable +private fun FieldRow( + label: String, + value: String, + onValueChange: (String) -> Unit, + modifier: Modifier = Modifier, + position: Position = Position.Alone, + placeholder: String = "", + error: String? = null, + hint: String? = null, + keyboardType: KeyboardType = KeyboardType.Text, + onImeAction: (() -> Unit)? = null, +) { + // ⚠️ KeyboardType.Password only tells the IME to drop suggestions; it does + // not mask anything. Without the transformation the app password renders in + // the clear on screen. + val masked = keyboardType == KeyboardType.Password + Column(modifier) { + GroupedSurface(position = position) { + Column(Modifier.fillMaxWidth().padding(horizontal = 16.dp, vertical = 12.dp)) { + Text( + label, + style = MaterialTheme.typography.labelMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + InlineTextField( + value = value, + onValueChange = onValueChange, + placeholder = placeholder, + keyboardType = keyboardType, + // A server address and a password are both case-sensitive, + // and sentence-casing either is a support ticket. + capitalization = KeyboardCapitalization.None, + imeAction = ImeAction.Go, + onImeAction = onImeAction, + visualTransformation = if (masked) { + PasswordVisualTransformation() + } else { + VisualTransformation.None + }, + ) + } + } + // ⚠️ Outside the surface, not inside it. An error rendered within the + // field's own card reads as part of the value the user typed. + (error ?: hint)?.let { + Text( + it, + style = MaterialTheme.typography.bodySmall, + color = if (error != null) { + MaterialTheme.colorScheme.error + } else { + MaterialTheme.colorScheme.onSurfaceVariant + }, + modifier = Modifier.padding(start = 16.dp, end = 16.dp, top = 6.dp), + ) + } + } +} + +/** + * The step's call to action, pinned at the bottom the way the onboarding shell + * puts it. + * + * One primary action per step, always in the same place, so the flow reads as + * one screen advancing rather than four different ones. A step that commits + * nothing renders no button rather than a disabled one. + */ +@Composable +private fun ColumnScope.StepActions(state: AddAccountUiState, viewModel: AddAccountViewModel) { + if (state.fatal != null) { + PrimaryAction( + label = stringResource(R.string.add_account_start_over), + enabled = true, + onClick = viewModel::onStartOver, ) - Button( - onClick = viewModel::onCredentialsSubmitted, + return + } + + when (val step = state.step) { + is AddAccountStep.EnterServer -> PrimaryAction( + label = stringResource(R.string.add_account_continue), + enabled = step.input.isNotBlank(), + onClick = viewModel::onServerSubmitted, + ) + + is AddAccountStep.EnterCredentials -> PrimaryAction( + label = stringResource(R.string.add_account_sign_in), enabled = step.username.isNotBlank() && step.password.isNotEmpty(), - modifier = Modifier.fillMaxWidth(), - ) { Text(stringResource(R.string.add_account_sign_in)) } + onClick = viewModel::onCredentialsSubmitted, + ) + + is AddAccountStep.ChooseLists -> PrimaryAction( + // The count is the feedback: a disabled button with no explanation + // is the commonest way a picker looks broken. + label = if (step.selected.isEmpty()) { + stringResource(R.string.add_account_no_lists_selected) + } else { + stringResource(R.string.add_account_save) + }, + enabled = step.selected.isNotEmpty(), + onClick = viewModel::onSave, + ) + + is AddAccountStep.WaitingForBrowser -> { + // A failed flow gets the primary action: there is nothing left to + // wait for, and the way forward is a password. + if (step.error != null) { + PrimaryAction( + label = stringResource(R.string.add_account_browser_use_password), + enabled = true, + onClick = viewModel::onBrowserCancelled, + ) + } else { + TextButton( + onClick = viewModel::onBrowserCancelled, + modifier = Modifier.fillMaxWidth(), + ) { Text(stringResource(R.string.add_account_browser_use_password)) } + } + } + + is AddAccountStep.Working, AddAccountStep.Done -> Unit + } +} + +/** The shell's primary action shape, so every step ends identically. */ +@Composable +private fun PrimaryAction(label: String, enabled: Boolean, onClick: () -> Unit) { + Button( + onClick = onClick, + enabled = enabled, + modifier = Modifier.fillMaxWidth().height(56.dp), + ) { + Text(label, style = MaterialTheme.typography.titleMedium) } } @Composable private fun BrowserStep(step: AddAccountStep.WaitingForBrowser, viewModel: AddAccountViewModel) { Column(Modifier.padding(horizontal = 16.dp), verticalArrangement = Arrangement.spacedBy(16.dp)) { + // ⚠️ The title changes too. Swapping only the body left every failure in + // this phase — a burnt credential, a 429, a maintenance page — sitting + // under the heading "Waiting for your browser", which reads as "still + // working" when nothing is working and nothing else will happen. Message( icon = { if (step.error == null) { @@ -195,7 +461,11 @@ private fun BrowserStep(step: AddAccountStep.WaitingForBrowser, viewModel: AddAc Icon(Icons.Rounded.CloudOff, contentDescription = null) } }, - title = stringResource(R.string.add_account_browser_title), + title = if (step.error == null) { + stringResource(R.string.add_account_browser_title) + } else { + stringResource(R.string.add_account_browser_failed_title) + }, text = step.error ?: stringResource(R.string.add_account_browser_body), ) @@ -209,10 +479,6 @@ private fun BrowserStep(step: AddAccountStep.WaitingForBrowser, viewModel: AddAc ) } - TextButton( - onClick = viewModel::onBrowserCancelled, - modifier = Modifier.fillMaxWidth(), - ) { Text(stringResource(R.string.add_account_browser_use_password)) } } } @@ -230,11 +496,7 @@ private fun WorkingStep(step: AddAccountStep.Working) { @Composable private fun ListsStep(step: AddAccountStep.ChooseLists, viewModel: AddAccountViewModel) { - Text( - stringResource(R.string.add_account_lists_title), - style = MaterialTheme.typography.titleMedium, - modifier = Modifier.padding(horizontal = 16.dp, vertical = 8.dp), - ) + // The heading is the shell's; this step only lists. step.collections.forEachIndexed { index, collection -> val checked = collection.url in step.selected GroupedRow( @@ -246,27 +508,13 @@ private fun ListsStep(step: AddAccountStep.ChooseLists, viewModel: AddAccountVie }, position = positionOf(index, step.collections.size), selected = checked, - modifier = Modifier.padding(horizontal = 16.dp), - trailing = { - Checkbox(checked = checked, onCheckedChange = null) - }, + // The family marks a chosen row with a check, not a Material + // checkbox — same affordance every picker in the app uses. + trailing = { if (checked) SelectedCheck() }, onClick = { viewModel.onListToggled(collection.url) }, ) } Spacer(Modifier.height(16.dp)) - Button( - onClick = viewModel::onSave, - enabled = step.selected.isNotEmpty(), - modifier = Modifier.fillMaxWidth().padding(horizontal = 16.dp), - ) { - Text( - if (step.selected.isEmpty()) { - stringResource(R.string.add_account_no_lists_selected) - } else { - stringResource(R.string.add_account_save) - }, - ) - } } @Composable diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt index 12ea333..f565f80 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt @@ -25,7 +25,11 @@ import javax.inject.Inject sealed interface AddAccountStep { /** Type an address. The whole flow starts from one field. */ - data class EnterServer(val input: String = "", val error: String? = null) : AddAccountStep + data class EnterServer( + val input: String = "", + /** A cause, not a message — the screen owns the wording. */ + val error: CalDavDiscovery.Outcome.Cause? = null, + ) : AddAccountStep data class Working(val message: String) : AddAccountStep @@ -134,9 +138,9 @@ class AddAccountViewModel @Inject constructor( // would make it permanently unreachable. CalDavDiscovery.Outcome.Unauthenticated -> offerSignIn() - is CalDavDiscovery.Outcome.NotCalDav -> backToServer(outcome.reason) + is CalDavDiscovery.Outcome.NotCalDav -> backToServer(outcome.cause) - is CalDavDiscovery.Outcome.Failed -> backToServer(outcome.reason) + is CalDavDiscovery.Outcome.Failed -> backToServer(outcome.cause) } } } @@ -171,15 +175,15 @@ class AddAccountViewModel @Inject constructor( ) } - is CalDavDiscovery.Outcome.NotCalDav -> backToServer(outcome.reason) - is CalDavDiscovery.Outcome.Failed -> backToServer(outcome.reason) + is CalDavDiscovery.Outcome.NotCalDav -> backToServer(outcome.cause) + is CalDavDiscovery.Outcome.Failed -> backToServer(outcome.cause) } } } /** Step 2b → the browser flow, if this looks like a Nextcloud. */ private suspend fun offerSignIn() { - val root = serverRoot ?: return backToServer("Could not work out the server address.") + val root = serverRoot ?: return backToServer(CalDavDiscovery.Outcome.Cause.NOT_AN_ADDRESS) val flow = gateway.startLoginFlow(root) if (flow == null) { @@ -230,7 +234,7 @@ class AddAccountViewModel @Inject constructor( if (outcome is CalDavDiscovery.Outcome.Found) { onDiscovered(outcome) } else { - backToServer("Signed in, but no task lists could be read.") + backToServer(CalDavDiscovery.Outcome.Cause.NO_CALENDARS) } return@launch } @@ -371,8 +375,16 @@ class AddAccountViewModel @Inject constructor( it.copy(step = AddAccountStep.EnterServer(input = typedInput), fatal = reason) } - private fun backToServer(reason: String) = _state.update { - it.copy(step = AddAccountStep.EnterServer(input = typedInput, error = reason)) + /** + * ⚠️ Carries the [CalDavDiscovery.Outcome.Cause], never the detail string. + * + * The detail is the server's own words — routinely a bare status line, often + * in a language the user does not read, and always outside `strings.xml`. + * The screen turns the cause into a translated sentence; nothing renders the + * detail. + */ + private fun backToServer(cause: CalDavDiscovery.Outcome.Cause) = _state.update { + it.copy(step = AddAccountStep.EnterServer(input = typedInput, error = cause)) } private fun browserFailed(reason: String) = _state.update { diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/licences/LicencesScreen.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/licences/LicencesScreen.kt index 5a3792b..84c86ab 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/licences/LicencesScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/licences/LicencesScreen.kt @@ -12,6 +12,7 @@ import androidx.compose.ui.res.stringResource import androidx.compose.ui.unit.dp import de.jeanlucmakiola.agendula.R import de.jeanlucmakiola.floret.components.CollapsingScaffold +import de.jeanlucmakiola.floret.components.GroupedListInset import de.jeanlucmakiola.floret.components.GroupedRow import de.jeanlucmakiola.floret.components.positionOf @@ -35,7 +36,7 @@ internal fun LicencesScreen(onBack: () -> Unit) { stringResource(R.string.licences_intro), style = MaterialTheme.typography.bodyMedium, color = MaterialTheme.colorScheme.onSurfaceVariant, - modifier = Modifier.padding(horizontal = 16.dp), + modifier = Modifier.padding(horizontal = GroupedListInset), ) Spacer(Modifier.height(16.dp)) @@ -44,7 +45,6 @@ internal fun LicencesScreen(onBack: () -> Unit) { title = attribution.name, summary = "${attribution.copyright} · ${attribution.licence.spdxId}", position = positionOf(index, OpenSourceLicences.ALL.size), - modifier = Modifier.padding(horizontal = 16.dp), onClick = { uriHandler.openUri(attribution.sourceUrl) }, ) } @@ -54,7 +54,7 @@ internal fun LicencesScreen(onBack: () -> Unit) { stringResource(R.string.licences_footer), style = MaterialTheme.typography.bodySmall, color = MaterialTheme.colorScheme.onSurfaceVariant, - modifier = Modifier.padding(horizontal = 16.dp), + modifier = Modifier.padding(horizontal = GroupedListInset), ) Spacer(Modifier.height(24.dp)) } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/settings/SettingsScreen.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/settings/SettingsScreen.kt index 7c6f61a..91dfb01 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/settings/SettingsScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/settings/SettingsScreen.kt @@ -99,6 +99,7 @@ import de.jeanlucmakiola.floret.identity.expandEnter import de.jeanlucmakiola.floret.reminders.ReminderOverride import de.jeanlucmakiola.floret.reminders.reminderOverrideFor import de.jeanlucmakiola.agendula.ui.common.OnResume +import de.jeanlucmakiola.agendula.ui.accounts.AccountDetailScreen import de.jeanlucmakiola.agendula.ui.accounts.AccountsScreen import de.jeanlucmakiola.agendula.ui.accounts.AccountsViewModel import de.jeanlucmakiola.agendula.ui.accounts.AddAccountScreen @@ -113,14 +114,15 @@ private enum class SettingsSection { Export, Accounts, AddAccount, + Account, Licences, ; - /** Where back goes: Export is opened from Storage, AddAccount from Accounts. */ + /** Where back goes: Export is opened from Storage, the account screens from Accounts. */ val parent: SettingsSection? get() = when (this) { Export -> Storage - AddAccount -> Accounts + AddAccount, Account -> Accounts else -> null } } @@ -148,6 +150,8 @@ fun SettingsScreen( var section by rememberSaveable { mutableStateOf(null) } // Hoisted so the add flow can refresh the list it returns to. val accountsViewModel: AccountsViewModel = hiltViewModel() + // Which account the detail screen is showing; the section alone cannot say. + var openAccount by rememberSaveable { mutableStateOf(null) } // Inside a sub-screen, system back (button or gesture) returns to the hub // rather than popping the whole Settings destination to the lists overview. @@ -189,6 +193,10 @@ fun SettingsScreen( SlideInSection(visible = accountsOpen) { AccountsScreen( onAddAccount = { section = SettingsSection.AddAccount }, + onOpenAccount = { + openAccount = it + section = SettingsSection.Account + }, onBack = { section = null }, viewModel = accountsViewModel, ) @@ -196,15 +204,24 @@ fun SettingsScreen( SlideInSection(visible = section == SettingsSection.Licences) { LicencesScreen(onBack = { section = null }) } + SlideInSection(visible = section == SettingsSection.Account) { + openAccount?.let { id -> + AccountDetailScreen( + accountId = id, + onBack = { section = SettingsSection.Accounts }, + onRemoved = { section = SettingsSection.Accounts }, + onSignInAgain = { section = SettingsSection.AddAccount }, + viewModel = accountsViewModel, + ) + } + } SlideInSection(visible = section == SettingsSection.AddAccount) { AddAccountScreen( - onDone = { - // The list stays composed underneath, so nothing re-runs its - // init and OnResume never fires on a section change — without - // this the user returns to the empty state they just left. - accountsViewModel.refresh() - section = SettingsSection.Accounts - }, + // The list stays composed underneath, so nothing re-runs its + // init and OnResume never fires on a section change. It no longer + // needs to: the accounts come from an observed query, so a new + // account — and every later sync — arrives on its own. + onDone = { section = SettingsSection.Accounts }, onBack = { section = SettingsSection.Accounts }, ) } diff --git a/app/src/main/res/drawable/ic_provider_nextcloud.xml b/app/src/main/res/drawable/ic_provider_nextcloud.xml new file mode 100644 index 0000000..7c089d9 --- /dev/null +++ b/app/src/main/res/drawable/ic_provider_nextcloud.xml @@ -0,0 +1,24 @@ + + + + + + + diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 7a4bd50..bc573ba 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -309,15 +309,33 @@ Add a CalDAV account and Agendula keeps your task lists in step with it — Nextcloud, Radicale, Baïkal and anything else that speaks the protocol. Add an account Remove account - Remove %1$s? - Its task lists stay on this device and stop syncing. Nothing is deleted from the server. - Remove + %1$s stops syncing with this device. Nothing is deleted from the server. + Remove, keep the tasks + Its lists stay on this device as device-only lists. + Remove and delete the tasks + Also deletes this account\u2019s lists and tasks from this device. Never synced + Last sync + CalDAV account Last sync didn\u2019t finish Sync now + Sign-in didn\u2019t finish + Step %1$d of %2$d + Where are your tasks? + Enter the address of your CalDAV server, or the email address you use with it. + Sign in + Use an app password if your provider offers one \u2014 it can be revoked without changing your account password. + Pick the task lists to keep in sync. You can change this later. + That doesn\u2019t look like a web address. Try something like cloud.example.com. + That address answered, but it isn\u2019t a CalDAV server. If you pasted the website, try the server address your provider gives for calendars. + That server doesn\u2019t offer calendars. Check the address with your provider. + Couldn\u2019t reach that server. Check the address and your connection. + That address is unencrypted. Agendula only sends your password over https. + Signed in, but this account has no task lists. + The server ran into a problem. Try again in a moment. Sign in again to keep syncing Sign in again - Also delete this account\u2019s tasks from this device + %1$s \u00b7 %2$s Syncing Syncing tasks diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt index 1bc8a66..5f58406 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt @@ -85,7 +85,10 @@ class AddAccountViewModelTest { @Test fun `a server that is not CalDAV says so on the address step`() = runTest(dispatcher) { - gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NotCalDav("no calendar-access") + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NotCalDav( + CalDavDiscovery.Outcome.Cause.NO_CALENDAR_SUPPORT, + "no calendar-access", + ) val vm = viewModel() vm.onServerInputChanged("https://example.com/") @@ -93,7 +96,9 @@ class AddAccountViewModelTest { advanceUntilIdle() val step = vm.state.value.step as AddAccountStep.EnterServer - assertThat(step.error).contains("calendar-access") + // ⚠️ A cause, never the server's own words — the screen owns the + // wording, and a raw status line bypasses strings.xml entirely. + assertThat(step.error).isEqualTo(CalDavDiscovery.Outcome.Cause.NO_CALENDAR_SUPPORT) // The input survives, so the user can correct it rather than retype it. assertThat(step.input).isEqualTo("https://example.com/") } @@ -333,7 +338,10 @@ class AddAccountViewModelTest { ): CalDavDiscovery.Outcome { discoveries += Call(target, credentials) return discoveryOutcomes.removeFirstOrNull() - ?: CalDavDiscovery.Outcome.Failed("no outcome queued") + ?: CalDavDiscovery.Outcome.Failed( + CalDavDiscovery.Outcome.Cause.SERVER_ERROR, + "no outcome queued", + ) } override suspend fun startLoginFlow(server: HttpUrl) = loginFlow diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt index 8218edb..8e310ca 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt @@ -3,10 +3,12 @@ package de.jeanlucmakiola.caldav import at.bitfire.dav4jvm.DavResource import at.bitfire.dav4jvm.Response import at.bitfire.dav4jvm.property.CalendarHomeSet +import at.bitfire.dav4jvm.exception.HttpException import at.bitfire.dav4jvm.property.CurrentUserPrincipal import okhttp3.HttpUrl import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import okhttp3.OkHttpClient +import java.io.IOException /** * RFC 6764 discovery: from what the user typed to the list of task collections. @@ -75,9 +77,61 @@ class CalDavDiscovery( /** A 200 whose body says the credentials were not accepted (RFC 5397 §3). */ data object Unauthenticated : Outcome - data class NotCalDav(val reason: String) : Outcome + data class NotCalDav(val cause: Cause, val detail: String) : Outcome - data class Failed(val reason: String) : Outcome + data class Failed(val cause: Cause, val detail: String) : Outcome + + /** + * Why discovery ended, in a form the UI can translate. + * + * ⚠️ The UI must render *this*, never [Failed.detail]. A server's own + * words are untranslatable, frequently in a language the user does not + * read, and quite often a bare status line — "HTTP 405 Method Not + * Allowed" tells someone entering their address precisely nothing, and + * bypasses `strings.xml` entirely. [detail] exists for logs and bug + * reports, and is never shown. + */ + enum class Cause { + /** The address is not a URL, or names nothing we can look up. */ + NOT_AN_ADDRESS, + + /** Reached something, but it does not speak WebDAV at all. */ + NOT_A_DAV_SERVER, + + /** Speaks WebDAV but not CalDAV — a file-sharing endpoint, say. */ + NO_CALENDAR_SUPPORT, + + /** Nothing answered: DNS, connection refused, TLS, timeout. */ + UNREACHABLE, + + /** The address, or where it redirects, is plain HTTP. */ + INSECURE, + + /** Signed in, but the account exposes no calendar home. */ + NO_CALENDARS, + + /** The server answered, and the answer was an error of its own. */ + SERVER_ERROR, + } + } + + /** + * Classifies a transport or protocol failure for the UI. + * + * ⚠️ **405 is the interesting one.** It is what an ordinary web server + * answers to `PROPFIND`, which makes it the single most likely response to + * someone typing their *website* instead of their CalDAV address — and it + * means exactly "this is not a DAV server". Surfacing it as "HTTP 405 Method + * Not Allowed" hands the user a status code where they needed a sentence. + */ + private fun causeOf(error: Throwable): Outcome.Cause = when { + error is HttpException -> when (error.code) { + METHOD_NOT_ALLOWED, NOT_IMPLEMENTED, NOT_FOUND -> Outcome.Cause.NOT_A_DAV_SERVER + else -> Outcome.Cause.SERVER_ERROR + } + // Everything that never got an answer: DNS, refused, TLS, timeout. + error is IOException -> Outcome.Cause.UNREACHABLE + else -> Outcome.Cause.SERVER_ERROR } /** @@ -91,16 +145,18 @@ class CalDavDiscovery( ServiceDiscovery.asBaseUrl(input)?.let { typed -> if (!typed.isHttps && !allowCleartext) { return Outcome.Failed( - "\"$input\" is an unencrypted http:// address. Credentials are never sent " + - "over cleartext, so this can only ever answer \"not authorised\".", + Outcome.Cause.INSECURE, + "\"$input\" is an unencrypted http:// address", ) } } val candidates = ServiceDiscovery.candidatesFor(input, dns) - if (candidates.isEmpty()) return Outcome.Failed("could not read \"$input\" as an address or URL") + if (candidates.isEmpty()) { + return Outcome.Failed(Outcome.Cause.NOT_AN_ADDRESS, "no candidates for \"$input\"") + } - var lastFailure: Outcome = Outcome.Failed("no candidate answered") + var lastFailure: Outcome = Outcome.Failed(Outcome.Cause.UNREACHABLE, "no candidate answered") for (candidate in candidates) { when (val outcome = probe(candidate.url)) { is Outcome.Found, is Outcome.NeedsAuthentication, Outcome.Unauthenticated -> return outcome @@ -135,7 +191,7 @@ class CalDavDiscovery( // 401 is not a failure — iCloud and Zoho answer it from // /.well-known/caldav, which *is* the DAV root, and it is RFC-legal. if (isUnauthorized(error)) return Outcome.NeedsAuthentication(listOf(base.host)) - return Outcome.Failed(error.message ?: error.toString()) + return Outcome.Failed(causeOf(error), error.message ?: error.toString()) } // ⚠️ RFC 5397 §3: a 200 carrying means the @@ -148,17 +204,29 @@ class CalDavDiscovery( val href = principalHref ?: return if (davCapabilities.contains("calendar-access")) { - Outcome.Failed("server advertises calendar-access but returned no principal") + Outcome.Failed( + Outcome.Cause.NO_CALENDAR_SUPPORT, + "advertises calendar-access but returned no principal", + ) } else { - Outcome.NotCalDav("no DAV:current-user-principal, and no calendar-access in OPTIONS") + Outcome.NotCalDav( + Outcome.Cause.NOT_A_DAV_SERVER, + "no DAV:current-user-principal, and no calendar-access in OPTIONS", + ) } if (davCapabilities.isNotEmpty() && !davCapabilities.contains("calendar-access")) { - return Outcome.NotCalDav("OPTIONS advertises ${davCapabilities.joinToString()} but not calendar-access") + return Outcome.NotCalDav( + Outcome.Cause.NO_CALENDAR_SUPPORT, + "OPTIONS advertises ${davCapabilities.joinToString()} but not calendar-access", + ) } val principal = resource.location.resolve(href) - ?: return Outcome.Failed("principal href \"$href\" is not a usable URL") + ?: return Outcome.Failed( + Outcome.Cause.SERVER_ERROR, + "principal href \"$href\" is not a usable URL", + ) return fromPrincipal(principal, movedTo = resource.permanentLocation) } @@ -178,10 +246,12 @@ class CalDavDiscovery( } homeSetResult.exceptionOrNull()?.let { error -> if (isUnauthorized(error)) return Outcome.NeedsAuthentication(listOf(principal.host)) - return Outcome.Failed(error.message ?: error.toString()) + return Outcome.Failed(causeOf(error), error.message ?: error.toString()) } - if (homeSets.isEmpty()) return Outcome.Failed("principal has no calendar-home-set") + if (homeSets.isEmpty()) { + return Outcome.Failed(Outcome.Cause.NO_CALENDARS, "principal has no calendar-home-set") + } // Cross-host is legal and required, but never over plain HTTP: the // credentials follow the home set, and a downgrade would send them in the @@ -189,7 +259,10 @@ class CalDavDiscovery( val crossHost = homeSets.filter { it.host != principal.host } val insecure = homeSets.filter { principal.isHttps && !it.isHttps } if (insecure.isNotEmpty()) { - return Outcome.Failed("calendar-home-set downgrades to HTTP: ${insecure.first()}") + return Outcome.Failed( + Outcome.Cause.INSECURE, + "calendar-home-set downgrades to HTTP: ${insecure.first()}", + ) } val collections = linkedMapOf() @@ -228,6 +301,7 @@ class CalDavDiscovery( Outcome.NeedsAuthentication(needAuth.map { it.url.host }.distinct()) } else { Outcome.Failed( + Outcome.Cause.NO_CALENDARS, failures.firstOrNull()?.reason ?: "no calendar-home-set could be listed", ) } @@ -249,5 +323,9 @@ class CalDavDiscovery( companion object { /** `https://host/path` → the URL, or null. Convenience for callers. */ fun url(value: String): HttpUrl? = value.toHttpUrlOrNull() + + private const val NOT_FOUND = 404 + private const val METHOD_NOT_ALLOWED = 405 + private const val NOT_IMPLEMENTED = 501 } } diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavProvider.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavProvider.kt new file mode 100644 index 0000000..5422653 --- /dev/null +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavProvider.kt @@ -0,0 +1,91 @@ +package de.jeanlucmakiola.caldav + +import okhttp3.HttpUrl + +/** + * Which CalDAV service or server software an account talks to. + * + * Read off the two things an account already stores, so nothing extra is asked + * of the server and no column has to be added: the **host**, which names a + * hosted service outright, and the **principal URL's path**, whose shape is a + * fingerprint of the software behind it. + * + * [hosted] is what decides how an account is *named* on screen. A hosted + * service's host is boilerplate (`caldav.fastmail.com` for everyone), so its + * brand is the name; self-hosted software runs on the user's own host, which is + * the only thing that tells two of them apart. + */ +enum class CalDavProvider( + val label: String, + val hosted: Boolean, + internal val domains: Set = emptySet(), + internal val principalMarkers: Set = emptySet(), +) { + + /** + * ownCloud serves `/remote.php/dav/` too and cannot be told apart from here. + * Nextcloud is the far commoner of the two and the only one the add flow has + * a browser login for, so the mark goes to it — and because a self-hosted + * account is titled by its host, the name "Nextcloud" is never written next + * to an ownCloud server, only its mark. + */ + NEXTCLOUD("Nextcloud", hosted = false, principalMarkers = setOf("/remote.php/dav/")), + + BAIKAL("Baïkal", hosted = false, principalMarkers = setOf("/dav.php/")), + + DAVICAL("DAViCal", hosted = false, principalMarkers = setOf("/caldav.php/")), + + SOGO("SOGo", hosted = false, principalMarkers = setOf("/sogo/dav/")), + + FASTMAIL( + "Fastmail", + hosted = true, + domains = setOf("fastmail.com", "fastmail.fm", "messagingengine.com"), + ), + + ICLOUD("iCloud", hosted = true, domains = setOf("icloud.com", "me.com", "mac.com")), + + GOOGLE("Google", hosted = true, domains = setOf("gmail.com", "googlemail.com", "google.com")), + + MAILBOX_ORG("mailbox.org", hosted = true, domains = setOf("mailbox.org")), + + POSTEO("Posteo", hosted = true, domains = setOf("posteo.de", "posteo.net")), + + ZOHO("Zoho", hosted = true, domains = setOf("zoho.com", "zoho.eu")), + + YANDEX("Yandex", hosted = true, domains = setOf("yandex.ru", "yandex.com")), + ; + + companion object { + + /** The service a host belongs to, if it is one we know by name. */ + fun forHost(host: String): CalDavProvider? { + val lower = host.lowercase().trimEnd('.') + return entries.firstOrNull { provider -> + provider.domains.any { lower == it || lower.endsWith(".$it") } + } + } + + /** The service behind an email address or a typed URL, if any. */ + fun forInput(input: String): CalDavProvider? { + val host = ServiceDiscovery.asBaseUrl(input)?.host + ?: ServiceDiscovery.domainOf(input) + ?: return null + return forHost(host) + } + + /** + * The provider behind a principal URL: the host first, because a service + * we know by name is not in doubt, then the path shape. + */ + fun forPrincipal(url: HttpUrl): CalDavProvider? = + forHost(url.host) ?: forPath(url.encodedPath) + + private fun forPath(path: String): CalDavProvider? { + val lower = path.lowercase() + return entries.firstOrNull { provider -> + provider.principalMarkers.any { it in lower } + } + } + } +} diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt index 1ed8fb7..506ed0e 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlow.kt @@ -162,14 +162,14 @@ class NextcloudLoginFlow( val root = json.parseToJsonElement(body).jsonObject val server = root["server"]?.jsonPrimitive?.content?.toHttpUrlOrNull() ?: error("no server URL in poll response") - // Scheme only. A host mismatch here must never discard the credentials: - // the 200 is returned exactly once — the server deletes the row inside - // poll() before returning — so throwing would burn the app password and - // force the user through the whole flow again. - requireSecureOrigin(flow.pollEndpoint, server) + // ⚠️ Coerced, never refused — neither the host nor the scheme may discard + // the credentials. The 200 is returned exactly once (the server deletes + // the row inside poll() before answering), so a throw here burns a live + // app password and leaves it dangling in the user's device list. + val secureServer = secureOrigin(flow.pollEndpoint, server) PollResult.Approved( Credentials( - server = server, + server = secureServer, // ⚠️ loginName is what the user typed — possibly an email, an // LDAP-derived value, or the right name in the wrong case. It is // the Basic auth username and nothing else. Interpolating it into @@ -182,17 +182,14 @@ class NextcloudLoginFlow( }.getOrElse { PollResult.Failed(it.message ?: it.toString()) } /** - * The endpoint is generated from `overwrite.cli.url` / `overwriteprotocol` / + * These URLs are generated from `overwrite.cli.url` / `overwriteprotocol` / * `trusted_proxies`, which are misconfigured on a large fraction of - * self-hosted installs — so it is validated rather than trusted verbatim. + * self-hosted installs — so they are validated rather than trusted verbatim. * - * A downgrade to `http` is refused outright: the poll token is exchanged for a - * long-lived app password, which makes it a credential-grade secret. - */ - /** - * A downgrade to `http` is **fatal**. The poll token is exchanged for a - * long-lived app password and the login URL takes the account password, so - * both are credential-grade. + * A downgrade to `http` is **fatal here and only here**: this runs *before* + * the user has approved anything, and the login URL is where they type their + * account password. There is nothing to lose by refusing, and everything to + * lose by not. */ internal fun requireSecureOrigin(expected: HttpUrl, actual: HttpUrl) { if (expected.isHttps && !actual.isHttps) { @@ -200,6 +197,29 @@ class NextcloudLoginFlow( } } + /** + * The same downgrade, on the *poll response*, where refusing is the wrong + * answer. + * + * ⚠️ By this point the credential has already been issued, and Nextcloud + * returns it **exactly once** — the row is deleted inside `poll()` before it + * answers. Throwing here does not protect anything: it destroys a live app + * password, leaves one dangling in the user's device list, and sends them + * through the whole flow again. The comment on the host mismatch above says + * precisely this, and the scheme deserves the same treatment. + * + * Coercing is strictly safer than either alternative, because the invariant + * that matters is *what we do next*: we only ever talk to the coerced URL, so + * the credential never travels in cleartext regardless of what the server + * put in the JSON. + */ + internal fun secureOrigin(expected: HttpUrl, actual: HttpUrl): HttpUrl = + if (expected.isHttps && !actual.isHttps) { + actual.newBuilder().scheme("https").build() + } else { + actual + } + /** A different host than the user typed — reported, not refused. */ internal fun hostMismatchOf(expected: HttpUrl, actual: HttpUrl): HostMismatch? = if (expected.host != actual.host) HostMismatch(expected.host, actual.host) else null diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServerQuirks.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServerQuirks.kt index a78d7c5..868eeee 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServerQuirks.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServerQuirks.kt @@ -9,14 +9,17 @@ import okhttp3.HttpUrl * account password"* is the single most common support ticket any CalDAV client * inherits. Detecting it at account-add time by domain turns a dead end into one * sentence of instruction. + * + * The domains themselves live on [CalDavProvider] — one list, so a provider the + * accounts screen can mark is also a provider this can warn about. */ -enum class ServerQuirk(val domains: Set) { +enum class ServerQuirk { /** Fastmail: needs an app password, and CalDAV is not on the Basic plan. */ - FASTMAIL_APP_PASSWORD(setOf("fastmail.com", "fastmail.fm", "messagingengine.com")), + FASTMAIL_APP_PASSWORD, /** iCloud: app-specific password, and 2FA must be on to mint one. */ - ICLOUD_APP_SPECIFIC_PASSWORD(setOf("icloud.com", "me.com", "mac.com")), + ICLOUD_APP_SPECIFIC_PASSWORD, /** * Google: OAuth2-only, and it supports neither VTODO nor MKCALENDAR — its own @@ -24,7 +27,7 @@ enum class ServerQuirk(val domains: Set) { * drops it as a target, so this is a refusal with an explanation rather than * a 401 the user cannot act on. */ - GOOGLE_UNSUPPORTED(setOf("gmail.com", "googlemail.com", "google.com")), + GOOGLE_UNSUPPORTED, /** * Nextcloud's brute-force protection throttles then 429s **per source IP**, so @@ -32,27 +35,26 @@ enum class ServerQuirk(val domains: Set) { * clients on that network. Not domain-detectable; set when a server identifies * itself. Kept here so the engine has one place to ask. */ - NEXTCLOUD_BRUTE_FORCE_PROTECTED(emptySet()), + NEXTCLOUD_BRUTE_FORCE_PROTECTED, ; companion object { /** The quirk implied by an email address or a URL host, if any. */ - fun forInput(input: String): ServerQuirk? { - val host = ServiceDiscovery.asBaseUrl(input)?.host - ?: ServiceDiscovery.domainOf(input) - ?: return null - return forHost(host) - } + fun forInput(input: String): ServerQuirk? = CalDavProvider.forInput(input)?.quirk - fun forHost(host: String): ServerQuirk? { - val lower = host.lowercase().trimEnd('.') - return entries.firstOrNull { quirk -> - quirk.domains.any { lower == it || lower.endsWith(".$it") } - } - } + fun forHost(host: String): ServerQuirk? = CalDavProvider.forHost(host)?.quirk fun forUrl(url: HttpUrl): ServerQuirk? = forHost(url.host) + + private val CalDavProvider.quirk: ServerQuirk? + get() = when (this) { + CalDavProvider.FASTMAIL -> FASTMAIL_APP_PASSWORD + CalDavProvider.ICLOUD -> ICLOUD_APP_SPECIFIC_PASSWORD + CalDavProvider.GOOGLE -> GOOGLE_UNSUPPORTED + CalDavProvider.NEXTCLOUD -> NEXTCLOUD_BRUTE_FORCE_PROTECTED + else -> null + } } /** True when discovery should not even be attempted. */ diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt index 92a54de..7450ca9 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt @@ -71,9 +71,43 @@ object ServiceDiscovery { val trimmed = input.trim() if (trimmed.isEmpty()) return emptyList() - // A typed base URL is used as typed. PROPFIND on it can return principal, - // home-set and collection in one response, so DNS is never consulted. - asBaseUrl(trimmed)?.let { return listOf(Candidate(it, "base URL as typed")) } + // A typed URL is taken as typed and DNS is never consulted — a user who + // gave us an address meant it. + asBaseUrl(trimmed)?.let { typed -> + val candidates = mutableListOf() + // ⚠️ A typed URL is not automatically a *DAV* URL, and the bare + // origin is what people actually type. `https://cloud.example.com` + // is the web UI: PROPFIND on it returns the 405 any web server + // answers, which reads as "not a CalDAV server" about a perfectly + // good one. RFC 6764 §6 exists precisely for this case, so the + // well-known probe has to run here too — returning the typed URL as + // the only candidate is what made a working Nextcloud undiscoverable. + // + // ⚠️ Built through `newBuilder`, not by interpolating `host`, which + // returns an IPv6 literal *without* its brackets — "fd00::1", not + // "[fd00::1]". Pasting that back into a URL yields a string OkHttp + // will not parse, so both candidates would be dropped and a typed + // IPv6 address would report as "not an address". `toString()` also + // drops a default port on its own. + val origin = typed.newBuilder() + .encodedPath("/") + .query(null) + .fragment(null) + .build() + .toString() + if (typed.encodedPath.trim('/').isEmpty()) { + add(candidates, origin, WELL_KNOWN, "typed origin + .well-known") + add(candidates, origin, "/", "typed origin + root") + } else { + // A deep URL may well be the DAV root itself, and PROPFIND on it + // can return principal, home-set and collection in one response. + // The well-known stays as the fallback for a path that was a + // guess. + candidates += Candidate(typed, "base URL as typed") + add(candidates, origin, WELL_KNOWN, "typed host + .well-known") + } + return candidates + } val domain = domainOf(trimmed) ?: return emptyList() val candidates = mutableListOf() diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavDiscoveryTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavDiscoveryTest.kt index e727509..c26acce 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavDiscoveryTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavDiscoveryTest.kt @@ -191,7 +191,10 @@ class CalDavDiscoveryTest { // answer "not authorised" — which the user reads as a wrong password. val outcome = discovery.discover("http://cloud.example.com/dav/") assertThat(outcome).isInstanceOf(CalDavDiscovery.Outcome.Failed::class.java) - assertThat((outcome as CalDavDiscovery.Outcome.Failed).reason).contains("http://") + val failed = outcome as CalDavDiscovery.Outcome.Failed + // The cause is what the UI renders; the detail is for logs only. + assertThat(failed.cause).isEqualTo(CalDavDiscovery.Outcome.Cause.INSECURE) + assertThat(failed.detail).contains("http://") } @Test diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavProviderTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavProviderTest.kt new file mode 100644 index 0000000..66d479d --- /dev/null +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalDavProviderTest.kt @@ -0,0 +1,42 @@ +package de.jeanlucmakiola.caldav + +import com.google.common.truth.Truth.assertThat +import okhttp3.HttpUrl.Companion.toHttpUrl +import org.junit.Test + +class CalDavProviderTest { + + @Test + fun `a hosted service is known by its host`() { + assertThat(CalDavProvider.forInput("me@fastmail.com")).isEqualTo(CalDavProvider.FASTMAIL) + assertThat(CalDavProvider.forHost("caldav.icloud.com")).isEqualTo(CalDavProvider.ICLOUD) + assertThat(CalDavProvider.forHost("dav.mailbox.org")).isEqualTo(CalDavProvider.MAILBOX_ORG) + assertThat(CalDavProvider.forHost("cloud.example.de")).isNull() + } + + @Test + fun `self-hosted software is known by its principal path`() { + val nextcloud = "https://cloud.example.de/remote.php/dav/principals/users/jo/".toHttpUrl() + assertThat(CalDavProvider.forPrincipal(nextcloud)).isEqualTo(CalDavProvider.NEXTCLOUD) + + val baikal = "https://dav.example.de/dav.php/principals/jo/".toHttpUrl() + assertThat(CalDavProvider.forPrincipal(baikal)).isEqualTo(CalDavProvider.BAIKAL) + + val unknown = "https://dav.example.de/jo/".toHttpUrl() + assertThat(CalDavProvider.forPrincipal(unknown)).isNull() + } + + @Test + fun `the host wins over the path`() { + // Fastmail's principals sit under a path we know nothing about; the host + // already named the service, so nothing else is consulted. + val url = "https://caldav.fastmail.com/dav/principals/user/me@fastmail.com/".toHttpUrl() + assertThat(CalDavProvider.forPrincipal(url)).isEqualTo(CalDavProvider.FASTMAIL) + } + + @Test + fun `hosted services are titled by brand, self-hosted ones by host`() { + assertThat(CalDavProvider.FASTMAIL.hosted).isTrue() + assertThat(CalDavProvider.NEXTCLOUD.hosted).isFalse() + } +} diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt index 1059e7e..fcaf333 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/NextcloudLoginFlowTest.kt @@ -166,4 +166,42 @@ class NextcloudLoginFlowTest { assertThat(mismatch!!.actual).isEqualTo("cloud.example.com") assertThat(mismatch.message).contains("overwrite.cli.url") } + + @Test fun `a downgraded server URL in the poll response is coerced, not refused`() { + // ⚠️ The credential has already been issued and the 200 comes exactly + // once — the row is deleted inside poll() before it answers. Throwing + // here destroys a live app password and leaves it dangling in the user's + // device list, for no protection at all. + val flow = NextcloudLoginFlow(OkHttpClient(), "test") + val expected = "https://cloud.example.com/login/v2/poll".toHttpUrl() + val downgraded = "http://cloud.example.com".toHttpUrl() + + val coerced = flow.secureOrigin(expected, downgraded) + + assertThat(coerced.scheme).isEqualTo("https") + assertThat(coerced.host).isEqualTo("cloud.example.com") + } + + @Test fun `an already-secure server URL is left alone`() { + val flow = NextcloudLoginFlow(OkHttpClient(), "test") + val expected = "https://cloud.example.com/login/v2/poll".toHttpUrl() + val actual = "https://dav.example.com".toHttpUrl() + + // A different host is reported by hostMismatchOf, never rewritten here. + assertThat(flow.secureOrigin(expected, actual)).isEqualTo(actual) + } + + @Test fun `the login URL is still refused outright when downgraded`() { + // Before approval there is nothing to lose by refusing, and the login URL + // is where the *account* password gets typed. + val flow = NextcloudLoginFlow(OkHttpClient(), "test") + val failure = runCatching { + flow.requireSecureOrigin( + "https://cloud.example.com".toHttpUrl(), + "http://cloud.example.com/login".toHttpUrl(), + ) + }.exceptionOrNull() + + assertThat(failure).isNotNull() + } } diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServiceDiscoveryTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServiceDiscoveryTest.kt index 67b807a..c9a582e 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServiceDiscoveryTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServiceDiscoveryTest.kt @@ -24,8 +24,19 @@ class ServiceDiscoveryTest { @Test fun `a typed base URL is used as typed and never triggers DNS`() { val dns = FakeDns(srv = mapOf("_caldavs._tcp.example.com" to listOf(SrvRecord(0, 0, 8443, "dav.example.com")))) - assertThat(urls("https://cloud.example.com/remote.php/dav/", dns)) - .containsExactly("https://cloud.example.com/remote.php/dav/") + val candidates = urls("https://cloud.example.com/remote.php/dav/", dns) + + // The typed path is tried first and the SRV target is never consulted: + // a user who gave us an address meant it. + assertThat(candidates.first()).isEqualTo("https://cloud.example.com/remote.php/dav/") + assertThat(candidates).doesNotContain("https://dav.example.com:8443/") + // The well-known follows as a fallback, because a typed path may have + // been a guess — see `a typed deep URL is tried as typed, first`. + assertThat(candidates) + .containsExactly( + "https://cloud.example.com/remote.php/dav/", + "https://cloud.example.com/.well-known/caldav", + ).inOrder() } @Test @@ -125,4 +136,59 @@ class ServiceDiscoveryTest { assertThat(ServiceDiscovery.candidatesFor("me@gmx.net", dns).map { it.origin }) .contains("domain as typed + TXT path=/begenda/dav/users/") } + + // --- a typed URL still gets the RFC 6764 probe ------------------------- + + @Test fun `a typed bare origin still tries well-known`() { + // ⚠️ The case every user actually types. The origin is the *web UI*, and + // PROPFIND on it returns the 405 any web server answers — which reads as + // "not a CalDAV server" about a working Nextcloud. Found against a real + // server, not by reading. + val paths = ServiceDiscovery.candidatesFor("https://cloud.example.com") + .map { it.url.encodedPath } + + assertThat(paths).containsExactly("/.well-known/caldav", "/").inOrder() + } + + @Test fun `a trailing slash is still a bare origin`() { + val paths = ServiceDiscovery.candidatesFor("https://cloud.example.com/") + .map { it.url.encodedPath } + + assertThat(paths).containsExactly("/.well-known/caldav", "/").inOrder() + } + + @Test fun `a typed deep URL is tried as typed, first`() { + // A deep URL may be the DAV root itself, where one PROPFIND can return + // principal, home-set and collection together. + val paths = ServiceDiscovery.candidatesFor("https://cloud.example.com/remote.php/dav/") + .map { it.url.encodedPath } + + assertThat(paths.first()).isEqualTo("/remote.php/dav/") + // …but the path may have been a guess, so the probe stays as a fallback. + assertThat(paths).contains("/.well-known/caldav") + } + + @Test fun `a non-default port survives the origin rebuild`() { + val candidates = ServiceDiscovery.candidatesFor("https://cloud.example.com:8443") + + assertThat(candidates.map { it.url.toString() }) + .containsExactly( + "https://cloud.example.com:8443/.well-known/caldav", + "https://cloud.example.com:8443/", + ).inOrder() + } + + @Test fun `an IPv6 literal keeps its brackets through the origin rebuild`() { + // ⚠️ `HttpUrl.host` hands back "fd00::1", not "[fd00::1]", so an origin + // built by interpolating it is a string OkHttp will not parse — both + // candidates were silently dropped and a homelab address reported as + // "not an address". + val candidates = ServiceDiscovery.candidatesFor("https://[fd00::1]:8443") + + assertThat(candidates.map { it.url.toString() }) + .containsExactly( + "https://[fd00::1]:8443/.well-known/caldav", + "https://[fd00::1]:8443/", + ).inOrder() + } } diff --git a/dav/PROVENANCE.md b/dav/PROVENANCE.md index dc0dd17..d2f279d 100644 --- a/dav/PROVENANCE.md +++ b/dav/PROVENANCE.md @@ -155,3 +155,28 @@ exists in this version: Fetch the new tag, diff against `f434c9d`, reapply changes 1–4, run `./gradlew :dav:test`. If upstream's suite fails, the port is wrong — that is the entire reason it is vendored alongside the code. + +## Change 7 — a same-host HTTPS→HTTP redirect is upgraded, not refused + +`DavResource.followRedirects` threw `DavException("Received redirect from HTTPS +to HTTP")` for any downgrade. That is right for a redirect to a *different* host, +which has no innocent reading. It is wrong for the same host, and the same host +is the case that actually occurs. + +⚠️ **A Nextcloud behind a TLS-terminating reverse proxy without +`overwriteprotocol` — or without `proxy_set_header X-Forwarded-Proto $scheme` — +builds every redirect with `http://`.** That includes the `/.well-known/caldav` +hop RFC 6764 discovery depends on. The server is entirely functional: +`/remote.php/dav/` answers 401 over HTTPS exactly as it should. But discovery +refuses the downgrade, falls back to a `PROPFIND` on the web root, gets the 405 +an ordinary web server returns, and reports "not a CalDAV server" about a working +CalDAV server. + +Now: when the redirect target's host matches the current one, the scheme is put +back to `https` and the hop continues. Re-issuing the same host and path over TLS +is *strictly safer* than obeying the redirect as sent, and it preserves the +invariant that matters — credentials never travel in cleartext. A cross-host +downgrade still throws. + +Found against a real server, not by reading: `cloud.jeanlucmakiola.de` returns +`301 → http://cloud.jeanlucmakiola.de/remote.php/dav/`. diff --git a/dav/build.gradle.kts b/dav/build.gradle.kts index 039e247..2678fc6 100644 --- a/dav/build.gradle.kts +++ b/dav/build.gradle.kts @@ -33,4 +33,5 @@ dependencies { // vendoring safe, which is the same call provider/PROVENANCE.md made. testImplementation(libs.junit4) testImplementation(libs.okhttp.mockwebserver) + testImplementation(libs.okhttp.tls) } diff --git a/dav/src/main/kotlin/at/bitfire/dav4jvm/DavResource.kt b/dav/src/main/kotlin/at/bitfire/dav4jvm/DavResource.kt index 4c09bdf..be13c28 100644 --- a/dav/src/main/kotlin/at/bitfire/dav4jvm/DavResource.kt +++ b/dav/src/main/kotlin/at/bitfire/dav4jvm/DavResource.kt @@ -635,12 +635,34 @@ open class DavResource @JvmOverloads constructor( if (response.isRedirect) // handle 3xx Redirection response.use { - val target = it.header("Location")?.let { location.resolve(it) } + var target = it.header("Location")?.let { location.resolve(it) } if (target != null) { log.fine("Redirected, new location = $target") - if (location.isHttps && !target.isHttps) - throw DavException("Received redirect from HTTPS to HTTP") + if (location.isHttps && !target.isHttps) { + // ⚠️ A downgrade to the *same host* is a server + // misconfiguration, not an attack, and it is the + // single most common one in this space: a Nextcloud + // behind a TLS-terminating proxy without + // `overwriteprotocol` (or `X-Forwarded-Proto`) builds + // every redirect with http://, including the + // /.well-known/caldav hop that discovery depends on. + // Refusing it outright makes a perfectly good server + // undiscoverable, and the user cannot tell why. + // + // Re-issuing the same host and path over TLS is + // strictly safer than what we were asked to do, and + // preserves the invariant that actually matters: + // credentials never travel in the clear. A downgrade + // pointing at a *different* host has no innocent + // reading, so that still fails. + if (target.host == location.host) { + target = target.newBuilder().scheme("https").build() + log.fine("Upgraded same-host downgrade back to $target") + } else { + throw DavException("Received redirect from HTTPS to HTTP") + } + } if (chainStillPermanent && (it.code == HTTP_MOVED_PERM || it.code == HTTP_PERM_REDIRECT)) permanentLocation = target diff --git a/dav/src/test/kotlin/at/bitfire/dav4jvm/LocalChangesTest.kt b/dav/src/test/kotlin/at/bitfire/dav4jvm/LocalChangesTest.kt index 0907b2d..dbf9ec8 100644 --- a/dav/src/test/kotlin/at/bitfire/dav4jvm/LocalChangesTest.kt +++ b/dav/src/test/kotlin/at/bitfire/dav4jvm/LocalChangesTest.kt @@ -10,10 +10,13 @@ import at.bitfire.dav4jvm.property.CurrentUserPrincipal import okhttp3.OkHttpClient import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer +import okhttp3.tls.HandshakeCertificates +import okhttp3.tls.HeldCertificate import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertNull +import org.junit.Assert.assertThrows import org.junit.Assert.assertTrue import org.junit.Before import org.junit.Test @@ -242,4 +245,73 @@ class LocalChangesTest { TimeZone.setDefault(zone) } } + + // --- change 7: a same-host downgrade is upgraded, a cross-host one is not --- + + /** + * A TLS MockWebServer, because the branch under test only exists on HTTPS. + * + * Same trap as change 2's: a test that runs against plain HTTP here would + * pass whatever the code did, since `location.isHttps` gates the whole + * decision. + */ + private fun httpsServer(): Pair { + val certificate = HeldCertificate.Builder() + .addSubjectAlternativeName("localhost") + .build() + val serverCertificates = HandshakeCertificates.Builder() + .heldCertificate(certificate) + .build() + val clientCertificates = HandshakeCertificates.Builder() + .addTrustedCertificate(certificate.certificate) + .build() + + val tlsServer = MockWebServer().apply { + useHttps(serverCertificates.sslSocketFactory(), false) + start() + } + val client = OkHttpClient.Builder() + .followRedirects(false) + .sslSocketFactory( + clientCertificates.sslSocketFactory(), + clientCertificates.trustManager, + ) + .build() + return tlsServer to client + } + + @Test + fun `a same-host redirect to http is retried over https`() { + // ⚠️ Exactly what a Nextcloud behind a proxy without `overwriteprotocol` + // sends for /.well-known/caldav. Refusing it makes a working CalDAV + // server report as "not a CalDAV server". + val (tls, client) = httpsServer() + val resource = DavResource(client, tls.url("/.well-known/caldav")) + val downgrade = "http://${tls.hostName}:${tls.port}/remote.php/dav/" + + tls.enqueue(MockResponse().setResponseCode(301).setHeader("Location", downgrade)) + tls.enqueue(MockResponse().setResponseCode(200)) + + resource.head { } + + tls.takeRequest() + val followed = tls.takeRequest() + assertEquals("/remote.php/dav/", followed.path) + // The scheme was put back; the host and path are the server's own. + assertEquals("https", resource.location.scheme) + assertEquals("/remote.php/dav/", resource.location.encodedPath) + } + + @Test + fun `a cross-host redirect to http is still refused`() { + val (tls, client) = httpsServer() + val resource = DavResource(client, tls.url("/dav/")) + tls.enqueue( + MockResponse().setResponseCode(301) + .setHeader("Location", "http://elsewhere.example.com/dav/"), + ) + + // No innocent reading of this one, so it stays fatal. + assertThrows(at.bitfire.dav4jvm.exception.DavException::class.java) { resource.head { } } + } } diff --git a/docs/SYNC-PLAN.md b/docs/SYNC-PLAN.md index 433a363..9f33117 100644 --- a/docs/SYNC-PLAN.md +++ b/docs/SYNC-PLAN.md @@ -854,6 +854,28 @@ would have quarantined every Lightning-authored task permanently. was re-requested for ever with nothing counting it, and account removal blocked on a 30-second connect timeout before anything visible happened. +## Noted for later, not built + +**Sign in via the installed Nextcloud app.** If the user already has Nextcloud +Files on the device, they should be able to pick the account they are *already +signed into* rather than typing a server address and minting a password by hand +— which is the longest and most failure-prone part of the flow, and the one that +produces the 405s and "wrong password" dead ends this branch keeps working +around. + +Nextcloud's Android app exposes its accounts through the **Single Sign-On** +library (`nextcloud/Android-SingleSignOn`): an AIDL binding to the Files app, +which returns a per-app token after the user picks an account and approves. It +would slot in beside Login Flow v2 as a third path on the address step — +offered only when the Files app is actually installed, with the typed-address +flow unchanged as the fallback for everyone else. + +Deliberately not in this branch. It adds a GPL-licensed dependency and a second +credential *shape* (an SSO token that the Files app can revoke independently of +anything we store), and both deserve their own decision rather than being +absorbed into a chunk. Worth doing: it is the difference between "type your +server address" and "tap your account". + ## What moves to floret-kit Recorded here so the follow-up branch is a file move rather than a rediscovery. diff --git a/floret-kit b/floret-kit index e047a2b..3dc3448 160000 --- a/floret-kit +++ b/floret-kit @@ -1 +1 @@ -Subproject commit e047a2bd48e6ea9cdc0d04f7dc9df311869ff2f1 +Subproject commit 3dc34482654a5268ce9ce9b30808515d37f18a43 diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 0f1f39d..a3987ba 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -156,6 +156,8 @@ androidx-test-rules = { group = "androidx.test", name = "rules", version.ref = " # :dav — vendored dav4jvm (see dav/PROVENANCE.md) okhttp = { group = "com.squareup.okhttp3", name = "okhttp", version.ref = "okhttp" } okhttp-mockwebserver = { group = "com.squareup.okhttp3", name = "mockwebserver", version.ref = "okhttp" } +# Test-only: MockWebServer over TLS, so an HTTPS-only code path can be exercised. +okhttp-tls = { group = "com.squareup.okhttp3", name = "okhttp-tls", version.ref = "okhttp" } xpp3 = { group = "org.ogce", name = "xpp3", version.ref = "xpp3" } # :caldav — RFC 6764 service discovery