From ba48a9821cf6e4add695b1c79418e7bee58a56fd Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Wed, 28 Oct 2026 14:49:49 +0100 Subject: [PATCH] Fix review findings on server trust (#68) Renewed pins can be re-trusted via sign in again, discovery keeps walking past an untrusted SRV target, trust is bound to the address it was given for, reauth replaces old pins, and the trust save is covered by rollback. --- .../agendula/data/sync/AccountRepository.kt | 31 +++- .../data/sync/PendingLoginFlowStore.kt | 19 +- .../agendula/data/sync/ServerTrustKeys.kt | 44 +++++ .../agendula/data/sync/ServerTrustStore.kt | 26 +-- .../ui/accounts/AccountDetailScreen.kt | 5 +- .../agendula/ui/accounts/AccountsScreen.kt | 2 +- .../agendula/ui/accounts/AccountsViewModel.kt | 14 +- .../ui/accounts/add/AddAccountScreen.kt | 18 +- .../ui/accounts/add/AddAccountViewModel.kt | 175 +++++++++++------- .../full/res/xml/network_security_config.xml | 3 + app/src/main/res/values/strings.xml | 1 + .../data/sync/HttpClientConstructionTest.kt | 41 ++++ .../accounts/add/AddAccountViewModelTest.kt | 78 +++++++- .../jeanlucmakiola/caldav/CalDavDiscovery.kt | 25 ++- .../de/jeanlucmakiola/caldav/CalDavHttp.kt | 18 +- .../jeanlucmakiola/caldav/ServiceDiscovery.kt | 31 +++- .../jeanlucmakiola/caldav/ServerTrustTest.kt | 48 +++++ 17 files changed, 445 insertions(+), 134 deletions(-) create mode 100644 app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustKeys.kt create mode 100644 app/src/testFull/java/de/jeanlucmakiola/agendula/data/sync/HttpClientConstructionTest.kt diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt index 9ab6c01..64eae5a 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt @@ -17,6 +17,7 @@ import kotlinx.coroutines.flow.first import kotlinx.coroutines.withContext import okhttp3.HttpUrl import okhttp3.HttpUrl.Companion.toHttpUrlOrNull +import java.io.IOException import javax.inject.Inject import javax.inject.Singleton @@ -193,7 +194,15 @@ class AccountRepository @Inject constructor( inserted += attach(id, selected) id } - serverTrust.put(accountId, trust) + // Inside the rollback's reach: a DataStore failure here would otherwise + // leave the row with no credential, the hazard the comment above names. + if (!saveTrust(accountId, trust)) { + rollback(accountId, inserted) + return@withContext Outcome.CredentialFailed( + Outcome.Cause.NOT_SAVED, + "the server trust could not be stored", + ) + } if (!credentials.put(accountId, appPassword)) { // Never leave a half-made account behind: without a credential it @@ -323,7 +332,8 @@ class AccountRepository @Inject constructor( -> Collections.NeedsSignIn is CalDavDiscovery.Outcome.NotCalDav -> Collections.Failed(outcome.cause) is CalDavDiscovery.Outcome.Failed -> Collections.Failed(outcome.cause) - // A certificate that changed since it was trusted; signing in again re-asks. + // A certificate that changed since it was trusted. The account offers + // "Sign in again" for a certificate failure, and that flow re-asks. is CalDavDiscovery.Outcome.UntrustedCertificate -> Collections.Failed(CalDavDiscovery.Outcome.Cause.TLS) } @@ -415,6 +425,11 @@ class AccountRepository @Inject constructor( "the device keystore would not store the password", ) } + // Before anything else changes, so a failure leaves the account stopped + // and the sign-in retryable rather than signed in with the old trust. + if (!withContext(NonCancellable) { saveTrust(accountId, trust) }) { + return Outcome.CredentialFailed(Outcome.Cause.NOT_SAVED, "the server trust could not be stored") + } withContext(NonCancellable) { database.runInTransaction { database.accounts().update( @@ -429,7 +444,6 @@ class AccountRepository @Inject constructor( ) attach(accountId, selected) } - serverTrust.put(accountId, trust) // ⚠️ Ticked lists are attached; unticked ones are left alone. The // picker pre-ticks everything *writable*, not everything already // attached, so detaching what is unticked would silently stop @@ -464,9 +478,18 @@ class AccountRepository @Inject constructor( inserted.forEach { database.taskLists().delete(it) } database.accounts().delete(accountId) } - serverTrust.forget(accountId) + // Ids are never reused, and a later create overwrites the keys anyway. + runCatching { serverTrust.forget(accountId) } } + private suspend fun saveTrust(accountId: Long, trust: ServerTrust): Boolean = + try { + serverTrust.put(accountId, trust) + true + } catch (_: IOException) { + false + } + /** * Puts every existing account back on the periodic schedule. * diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/PendingLoginFlowStore.kt b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/PendingLoginFlowStore.kt index efcf17c..8befbb8 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/PendingLoginFlowStore.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/PendingLoginFlowStore.kt @@ -2,11 +2,9 @@ package de.jeanlucmakiola.agendula.data.sync import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.Preferences -import androidx.datastore.preferences.core.booleanPreferencesKey import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.longPreferencesKey import androidx.datastore.preferences.core.stringPreferencesKey -import androidx.datastore.preferences.core.stringSetPreferencesKey import de.jeanlucmakiola.agendula.data.di.SyncStateDataStore import de.jeanlucmakiola.caldav.NextcloudLoginFlow import de.jeanlucmakiola.caldav.ServerTrust @@ -67,8 +65,7 @@ class PendingLoginFlowStore @Inject constructor( prefs[POLL_ENDPOINT] = flow.pollEndpoint.toString() prefs[POLL_TOKEN] = flow.pollToken prefs[DEADLINE] = flow.deadlineEpochSeconds - prefs[TRUSTED_CERTIFICATES] = trust.pinnedCertificates - prefs[CLEARTEXT] = trust.allowCleartext + TRUST.write(prefs, trust) } } @@ -79,8 +76,7 @@ class PendingLoginFlowStore @Inject constructor( prefs.remove(POLL_ENDPOINT) prefs.remove(POLL_TOKEN) prefs.remove(DEADLINE) - prefs.remove(TRUSTED_CERTIFICATES) - prefs.remove(CLEARTEXT) + TRUST.remove(prefs) } } @@ -145,20 +141,13 @@ class PendingLoginFlowStore @Inject constructor( ) } - private suspend fun trust(): ServerTrust { - val prefs = dataStore.data.first() - return ServerTrust( - pinnedCertificates = prefs[TRUSTED_CERTIFICATES].orEmpty(), - allowCleartext = prefs[CLEARTEXT] ?: false, - ) - } + private suspend fun trust(): ServerTrust = TRUST.read(dataStore.data.first()) private companion object { val LOGIN_URL = stringPreferencesKey("login_flow_url") val POLL_ENDPOINT = stringPreferencesKey("login_flow_poll_endpoint") val POLL_TOKEN = stringPreferencesKey("login_flow_poll_token") val DEADLINE = longPreferencesKey("login_flow_deadline") - val TRUSTED_CERTIFICATES = stringSetPreferencesKey("login_flow_trusted_certificates") - val CLEARTEXT = booleanPreferencesKey("login_flow_cleartext") + val TRUST = ServerTrustKeys("login_flow_trusted_certificates", "login_flow_cleartext") } } diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustKeys.kt b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustKeys.kt new file mode 100644 index 0000000..1a574a7 --- /dev/null +++ b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustKeys.kt @@ -0,0 +1,44 @@ +package de.jeanlucmakiola.agendula.data.sync + +import androidx.datastore.preferences.core.MutablePreferences +import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.booleanPreferencesKey +import androidx.datastore.preferences.core.stringSetPreferencesKey +import androidx.lifecycle.SavedStateHandle +import de.jeanlucmakiola.caldav.ServerTrust + +/** + * The one mapping between a [ServerTrust] and the key-value stores it is kept + * in, so a field added to it cannot be dropped by one of them. + */ +class ServerTrustKeys(private val certificatesName: String, private val cleartextName: String) { + + private val certificates = stringSetPreferencesKey(certificatesName) + private val cleartext = booleanPreferencesKey(cleartextName) + + fun read(prefs: Preferences) = ServerTrust( + pinnedCertificates = prefs[certificates].orEmpty(), + allowCleartext = prefs[cleartext] ?: false, + ) + + fun write(prefs: MutablePreferences, trust: ServerTrust) { + if (trust.pinnedCertificates.isEmpty()) { + prefs.remove(certificates) + } else { + prefs[certificates] = trust.pinnedCertificates + } + if (trust.allowCleartext) prefs[cleartext] = true else prefs.remove(cleartext) + } + + fun remove(prefs: MutablePreferences) = write(prefs, ServerTrust.DEFAULT) + + fun read(handle: SavedStateHandle) = ServerTrust( + pinnedCertificates = handle.get>(certificatesName).orEmpty().toSet(), + allowCleartext = handle[cleartextName] ?: false, + ) + + fun write(handle: SavedStateHandle, trust: ServerTrust) { + handle[certificatesName] = ArrayList(trust.pinnedCertificates) + handle[cleartextName] = trust.allowCleartext + } +} diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustStore.kt b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustStore.kt index 95efa49..364d759 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustStore.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustStore.kt @@ -2,9 +2,7 @@ package de.jeanlucmakiola.agendula.data.sync import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.Preferences -import androidx.datastore.preferences.core.booleanPreferencesKey import androidx.datastore.preferences.core.edit -import androidx.datastore.preferences.core.stringSetPreferencesKey import de.jeanlucmakiola.agendula.data.di.SyncStateDataStore import de.jeanlucmakiola.caldav.ServerTrust import kotlinx.coroutines.flow.Flow @@ -26,29 +24,15 @@ class ServerTrustStore @Inject constructor( dataStore.data.map { read(it, accountId) }.distinctUntilChanged() suspend fun put(accountId: Long, trust: ServerTrust) { - dataStore.edit { prefs -> - if (trust.pinnedCertificates.isEmpty()) { - prefs.remove(certificates(accountId)) - } else { - prefs[certificates(accountId)] = trust.pinnedCertificates - } - if (trust.allowCleartext) prefs[cleartext(accountId)] = true else prefs.remove(cleartext(accountId)) - } + dataStore.edit { keys(accountId).write(it, trust) } } suspend fun forget(accountId: Long) { - dataStore.edit { prefs -> - prefs.remove(certificates(accountId)) - prefs.remove(cleartext(accountId)) - } + dataStore.edit { keys(accountId).remove(it) } } - private fun read(prefs: Preferences, accountId: Long) = ServerTrust( - pinnedCertificates = prefs[certificates(accountId)].orEmpty(), - allowCleartext = prefs[cleartext(accountId)] ?: false, - ) + private fun read(prefs: Preferences, accountId: Long) = keys(accountId).read(prefs) - private fun certificates(accountId: Long) = stringSetPreferencesKey("server_trust_certs_$accountId") - - private fun cleartext(accountId: Long) = booleanPreferencesKey("server_trust_cleartext_$accountId") + private fun keys(accountId: Long) = + ServerTrustKeys("server_trust_certs_$accountId", "server_trust_cleartext_$accountId") } diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountDetailScreen.kt b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountDetailScreen.kt index 1d04595..36e815a 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountDetailScreen.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountDetailScreen.kt @@ -169,7 +169,7 @@ internal fun AccountDetailScreen( ) } } - if (shown.needsSignIn) { + if (shown.offersSignIn) { add { position -> GroupedRow( title = stringResource(R.string.accounts_sign_in_again), @@ -178,7 +178,8 @@ internal fun AccountDetailScreen( onClick = { onSignInAgain(account) }, ) } - } else { + } + if (!shown.needsSignIn) { add { position -> GroupedRow( title = stringResource(R.string.accounts_sync_now), diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt index a4467d5..f6567a5 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt @@ -119,7 +119,7 @@ internal fun AccountsScreen( when { row.removing -> CircularProgressIndicator(Modifier.size(20.dp)) !usable -> Unit - row.needsSignIn -> IconButton(onClick = { onSignInAgain(account) }) { + row.offersSignIn -> IconButton(onClick = { onSignInAgain(account) }) { Icon( Icons.AutoMirrored.Rounded.Login, contentDescription = stringResource(R.string.accounts_sign_in_again), diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt index ff6c475..6455e85 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt @@ -10,6 +10,7 @@ import de.jeanlucmakiola.agendula.data.sync.AccountRepository import de.jeanlucmakiola.agendula.data.sync.AccountStateStore import de.jeanlucmakiola.agendula.data.sync.ServerTrustStore import de.jeanlucmakiola.agendula.data.sync.SyncAvailability +import de.jeanlucmakiola.agendula.data.sync.SyncFailure import de.jeanlucmakiola.agendula.data.sync.SyncNotice import de.jeanlucmakiola.agendula.data.sync.SyncNoticeStore import de.jeanlucmakiola.agendula.data.sync.SyncTrigger @@ -191,7 +192,18 @@ class AccountsViewModel @Inject constructor( */ val notices: List = emptyList(), val lists: ListPush = ListPush.NONE, - ) + ) { + /** + * The last sync was refused over the server's certificate — typically a + * pinned one that was renewed. Nothing sets [needsSignIn] for that, yet + * signing in again is the only route that can trust the new one. + */ + val certificateRefused: Boolean + get() = account.lastSyncError?.let { SyncFailure.of(it).kind == SyncFailure.Kind.CERTIFICATE } == true + + /** Whether "Sign in again" is the way forward for this account. */ + val offersSignIn: Boolean get() = needsSignIn || certificateRefused + } /** * Accounts whose removal has started and not finished. diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountScreen.kt b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountScreen.kt index 4b96efb..5dfc4ae 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountScreen.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountScreen.kt @@ -440,11 +440,19 @@ private fun ColumnScope.StepActions(state: AddAccountUiState, viewModel: AddAcco ) { Text(stringResource(R.string.add_account_change_address)) } } - is AddAccountStep.TrustCertificate -> PrimaryAction( - label = stringResource(R.string.add_account_certificate_trust), - enabled = true, - onClick = viewModel::onCertificateTrusted, - ) + // The safe answer stays visible, as it does for plain http: back is + // not something every user knows to look for on a security prompt. + is AddAccountStep.TrustCertificate -> { + PrimaryAction( + label = stringResource(R.string.add_account_certificate_trust), + enabled = true, + onClick = viewModel::onCertificateTrusted, + ) + TextButton( + onClick = { viewModel.onBackWithin() }, + modifier = Modifier.fillMaxWidth(), + ) { Text(stringResource(R.string.add_account_certificate_decline)) } + } is AddAccountStep.WaitingForBrowser -> { // A failed flow is retried by default. With no browser a retry diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModel.kt b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModel.kt index 300de82..95bff6d 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModel.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModel.kt @@ -8,6 +8,7 @@ import de.jeanlucmakiola.agendula.data.sync.AccountCreator import de.jeanlucmakiola.agendula.data.sync.AccountRepository import de.jeanlucmakiola.agendula.data.sync.CalDavGateway import de.jeanlucmakiola.agendula.data.sync.LoginFlowRecord +import de.jeanlucmakiola.agendula.data.sync.ServerTrustKeys import de.jeanlucmakiola.caldav.CalDavDiscovery import de.jeanlucmakiola.caldav.CalDavProvider import de.jeanlucmakiola.caldav.NextcloudLoginFlow @@ -17,6 +18,8 @@ import de.jeanlucmakiola.caldav.ServerTrust import de.jeanlucmakiola.caldav.ServiceDiscovery import de.jeanlucmakiola.caldav.TaskCollection import de.jeanlucmakiola.agendula.data.di.ApplicationScope +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.withContext @@ -337,26 +340,33 @@ class AddAccountViewModel @Inject constructor( /** What the user agreed to trust in this flow, handed to every gateway call. */ private var trust: ServerTrust - get() = ServerTrust( - pinnedCertificates = handle.get>(KEY_TRUSTED_CERTIFICATES).orEmpty().toSet(), - allowCleartext = handle[KEY_CLEARTEXT] ?: false, - ) - set(value) { - handle[KEY_TRUSTED_CERTIFICATES] = ArrayList(value.pinnedCertificates) - handle[KEY_CLEARTEXT] = value.allowCleartext - } + get() = TRUST_KEYS.read(handle) + set(value) = TRUST_KEYS.write(handle, value) - /** The address plain http was accepted for; another address asks again. */ - private var cleartextAddress: String? - get() = handle[KEY_CLEARTEXT_ADDRESS] - set(value) { handle[KEY_CLEARTEXT_ADDRESS] = value } + /** + * The address [trust] was given for. Pins and plain http both belong to it, + * so submitting any other address starts from nothing. + */ + private var trustAddress: String? + get() = handle[KEY_TRUST_ADDRESS] + set(value) { handle[KEY_TRUST_ADDRESS] = value } + + /** + * The pins in [trust] came from the account being signed in to, not from + * this flow. The first certificate trusted now replaces them, so a renewed + * certificate retires the one it renews instead of joining it. + */ + private var inheritedPins: Boolean + get() = handle[KEY_INHERITED_PINS] ?: false + set(value) { handle[KEY_INHERITED_PINS] = value } /** Loading a re-authenticated account's stored trust, which discovery waits for. */ private var trustLoad: Job? = null - /** What [AddAccountStep.TrustCertificate] re-runs once trusted, and where Back goes. */ - private var trustRetry: (() -> Unit)? = null - private var trustDeclined: AddAccountStep? = null + /** The open [AddAccountStep.TrustCertificate] question, and where declining it goes. */ + private class TrustPrompt(val answer: CompletableDeferred, val back: AddAccountStep) + + private var trustPrompt: TrustPrompt? = null /** Which hosts asked for credentials, for the cross-domain diagnostic. */ private var hostsNeedingAuth: List @@ -461,7 +471,8 @@ class AddAccountViewModel @Inject constructor( trustLoad = viewModelScope.launch { val stored = runCatching { repository.trustFor(accountId) }.getOrDefault(ServerTrust.DEFAULT) trust = stored - if (stored.allowCleartext) cleartextAddress = address + trustAddress = address + inheritedPins = stored.pinnedCertificates.isNotEmpty() } } @@ -521,9 +532,6 @@ class AddAccountViewModel @Inject constructor( updateState { it.copy(originMismatch = null) } typedInput = input addressInput = input - trustRetry = null - trustDeclined = null - if (input != cleartextAddress) trust = trust.copy(allowCleartext = false) val quirk = ServerQuirk.forInput(input) if (quirk?.isFatal == true) { @@ -547,8 +555,18 @@ class AddAccountViewModel @Inject constructor( private fun discoverAddress() { working(AddAccountMessage.Progress.Discovering) viewModelScope.launch { + // ⚠️ After the stored trust has landed, not before: reset first and + // the load would put the old address's trust back on top. trustLoad?.join() - when (val outcome = gateway.discover(typedInput, trust = trust)) { + if (typedInput != trustAddress) { + trust = ServerTrust.DEFAULT + inheritedPins = false + trustAddress = typedInput + } + val outcome = discoverTrusting(addressStep(typedInput), AddAccountMessage.Progress.Discovering) { + gateway.discover(typedInput, trust = trust) + } ?: return@launch + when (outcome) { is CalDavDiscovery.Outcome.Found -> onDiscovered(outcome) is CalDavDiscovery.Outcome.NeedsAuthentication -> { @@ -570,8 +588,8 @@ class AddAccountViewModel @Inject constructor( backToAddressOrServer(outcome.cause) } - is CalDavDiscovery.Outcome.UntrustedCertificate -> - askToTrust(outcome.certificate, back = addressStep(typedInput)) { discoverAddress() } + // Asked about inside discoverTrusting, which only returns it declined. + is CalDavDiscovery.Outcome.UntrustedCertificate -> Unit } } } @@ -584,29 +602,49 @@ class AddAccountViewModel @Inject constructor( /** The warning is read and plain http is wanted anyway. */ fun onCleartextConfirmed() { if (current.step !is AddAccountStep.ConfirmCleartext) return - cleartextAddress = typedInput trust = trust.copy(allowCleartext = true) + trustAddress = typedInput discoverAddress() } /** The fingerprint is checked; pin it and re-run what it stopped. */ fun onCertificateTrusted() { val step = current.step as? AddAccountStep.TrustCertificate ?: return - val retry = trustRetry ?: return - trust = trust.withCertificate(step.certificate.fingerprint) - trustRetry = null - trustDeclined = null - retry() + val prompt = trustPrompt ?: return + trustPrompt = null + val fingerprint = step.certificate.fingerprint + trust = if (inheritedPins) { + trust.copy(pinnedCertificates = setOf(fingerprint.lowercase())) + } else { + trust.withCertificate(fingerprint) + } + inheritedPins = false + prompt.answer.complete(Unit) } - private fun askToTrust(certificate: ServerCertificate, back: AddAccountStep, retry: () -> Unit) { - // Already pinned and still refused: asking again could only loop. - if (certificate.fingerprint in trust.pinnedCertificates) { - return backToAddress(CalDavDiscovery.Outcome.Cause.TLS) + /** + * Runs [discover], and on a certificate the user could trust asks about it + * and runs it again once they have. Null when they declined; the step is + * then already [back]. + */ + private suspend fun discoverTrusting( + back: AddAccountStep, + progress: AddAccountMessage.Progress, + discover: suspend () -> CalDavDiscovery.Outcome, + ): CalDavDiscovery.Outcome? { + while (true) { + val outcome = discover() + if (outcome !is CalDavDiscovery.Outcome.UntrustedCertificate) return outcome + val prompt = TrustPrompt(CompletableDeferred(), back) + trustPrompt = prompt + updateState { it.copy(step = AddAccountStep.TrustCertificate(outcome.certificate), openInBrowser = null) } + try { + prompt.answer.await() + } catch (_: CancellationException) { + return null + } + working(progress) } - trustRetry = retry - trustDeclined = back - updateState { it.copy(step = AddAccountStep.TrustCertificate(certificate), openInBrowser = null) } } /** @@ -663,7 +701,11 @@ class AddAccountViewModel @Inject constructor( val root = serverRoot ?: return@launch backToAddress(CalDavDiscovery.Outcome.Cause.NOT_AN_ADDRESS) val credentials = CalDavGateway.Credentials(username, appPassword, root) - when (val outcome = gateway.discover(typedInput, credentials, trust)) { + val outcome = discoverTrusting( + AddAccountStep.EnterCredentials(username = username), + AddAccountMessage.Progress.SigningIn, + ) { gateway.discover(typedInput, credentials, trust) } ?: return@launch + when (outcome) { is CalDavDiscovery.Outcome.Found -> onDiscovered(outcome) // A second rejection is the point at which naming the provider's @@ -685,10 +727,7 @@ class AddAccountViewModel @Inject constructor( is CalDavDiscovery.Outcome.NotCalDav -> backToAddressOrServer(outcome.cause) is CalDavDiscovery.Outcome.Failed -> backToAddressOrServer(outcome.cause) - is CalDavDiscovery.Outcome.UntrustedCertificate -> askToTrust( - outcome.certificate, - back = AddAccountStep.EnterCredentials(username = username), - ) { discoverWithCredentials() } + is CalDavDiscovery.Outcome.UntrustedCertificate -> Unit } } } @@ -872,15 +911,17 @@ class AddAccountViewModel @Inject constructor( /** Discovery with a password the login flow just minted, at the server that issued it. */ private suspend fun discoverApproved(server: HttpUrl) { working(AddAccountMessage.Progress.ReadingLists) - val outcome = gateway.discover( - // The server the credentials were *issued by*, not the - // host the user typed — the two differ in exactly the - // host-mismatch case this flow already models, and - // getting it wrong burns the one-shot app password. - target = server.toString(), - credentials = CalDavGateway.Credentials(username, appPassword, server), - trust = trust, - ) + val outcome = discoverTrusting(addressStep(typedInput), AddAccountMessage.Progress.ReadingLists) { + gateway.discover( + // The server the credentials were *issued by*, not the + // host the user typed — the two differ in exactly the + // host-mismatch case this flow already models, and + // getting it wrong burns the one-shot app password. + target = server.toString(), + credentials = CalDavGateway.Credentials(username, appPassword, server), + trust = trust, + ) + } ?: return when (outcome) { is CalDavDiscovery.Outcome.Found -> onDiscovered(outcome) // ⚠️ Forward what actually happened. Reporting every @@ -909,10 +950,7 @@ class AddAccountViewModel @Inject constructor( CalDavDiscovery.Outcome.Unauthenticated -> backToAddress(CalDavDiscovery.Outcome.Cause.SERVER_ERROR) - is CalDavDiscovery.Outcome.UntrustedCertificate -> - askToTrust(outcome.certificate, back = addressStep(typedInput)) { - viewModelScope.launch { discoverApproved(server) } - } + is CalDavDiscovery.Outcome.UntrustedCertificate -> Unit } } @@ -1112,10 +1150,10 @@ class AddAccountViewModel @Inject constructor( } is AddAccountStep.TrustCertificate -> { - val back = trustDeclined ?: addressStep(typedInput) - trustRetry = null - trustDeclined = null - updateState { it.copy(step = back, fatal = null) } + val prompt = trustPrompt + trustPrompt = null + prompt?.answer?.cancel() + updateState { it.copy(step = prompt?.back ?: addressStep(typedInput), fatal = null) } true } @@ -1163,9 +1201,10 @@ class AddAccountViewModel @Inject constructor( trustLoad?.cancel() trustLoad = null trust = ServerTrust.DEFAULT - cleartextAddress = null - trustRetry = null - trustDeclined = null + trustAddress = null + inheritedPins = false + trustPrompt?.answer?.cancel() + trustPrompt = null current = AddAccountUiState() } @@ -1324,7 +1363,10 @@ class AddAccountViewModel @Inject constructor( ) viewModelScope.launch { val credentials = CalDavGateway.Credentials(username, appPassword, root) - when (val outcome = gateway.discover(target, credentials, trust)) { + val outcome = discoverTrusting(addressStep(addressInput), AddAccountMessage.Progress.ReadingLists) { + gateway.discover(target, credentials, trust) + } ?: return@launch + when (outcome) { is CalDavDiscovery.Outcome.Found -> onDiscovered(outcome, selection) is CalDavDiscovery.Outcome.NotCalDav -> backToAddressOrServer(outcome.cause) is CalDavDiscovery.Outcome.Failed -> backToAddressOrServer(outcome.cause) @@ -1335,8 +1377,7 @@ class AddAccountViewModel @Inject constructor( credentialsRejected() } CalDavDiscovery.Outcome.Unauthenticated -> credentialsRejected() - is CalDavDiscovery.Outcome.UntrustedCertificate -> - askToTrust(outcome.certificate, back = addressStep(addressInput)) { resumeDiscovery(hasSetup) } + is CalDavDiscovery.Outcome.UntrustedCertificate -> Unit } } } @@ -1566,13 +1607,19 @@ class AddAccountViewModel @Inject constructor( const val KEY_FALLBACK_EMAIL = "add_account.fallback_email" const val KEY_TRUSTED_CERTIFICATES = "add_account.trusted_certificates" const val KEY_CLEARTEXT = "add_account.cleartext" - const val KEY_CLEARTEXT_ADDRESS = "add_account.cleartext_address" + const val KEY_TRUST_ADDRESS = "add_account.trust_address" + const val KEY_INHERITED_PINS = "add_account.inherited_pins" + + val TRUST_KEYS = ServerTrustKeys(KEY_TRUSTED_CERTIFICATES, KEY_CLEARTEXT) /** What sends an email to the server field: no CalDAV on its domain, or nothing answering for it. */ val NO_SERVER_FOUND = setOf( CalDavDiscovery.Outcome.Cause.NOT_A_DAV_SERVER, CalDavDiscovery.Outcome.Cause.NO_CALENDAR_SUPPORT, CalDavDiscovery.Outcome.Cause.UNREACHABLE, + // The domain's own host would not set up TLS: usually its website, + // not a CalDAV server, so the server field is the way on. + CalDavDiscovery.Outcome.Cause.TLS, ) /** diff --git a/app/src/full/res/xml/network_security_config.xml b/app/src/full/res/xml/network_security_config.xml index 15e9a90..4e0774f 100644 --- a/app/src/full/res/xml/network_security_config.xml +++ b/app/src/full/res/xml/network_security_config.xml @@ -11,6 +11,9 @@ itself refuses an http:// URL for everyone else. This file cannot express a per-account choice, only a per-host one, and the hosts are the user's. + ⚠️ That makes CalDavHttp the only gate. A client built any other way would + speak http silently, so HttpClientConstructionTest fails the build on one. + ⚠️ KNOWN TRADE-OFF, and it is the widest form of this. base-config applies to *all* traffic, so any CA in the user store — a corporate MDM profile, a "free VPN" app's certificate, one installed during some earlier debugging — can diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 96cac40..eb37419 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -832,6 +832,7 @@ Issued by Valid until Trust this certificate + Don\u2019t trust it Which lists should sync? Read-only diff --git a/app/src/testFull/java/de/jeanlucmakiola/agendula/data/sync/HttpClientConstructionTest.kt b/app/src/testFull/java/de/jeanlucmakiola/agendula/data/sync/HttpClientConstructionTest.kt new file mode 100644 index 0000000..79c1ecf --- /dev/null +++ b/app/src/testFull/java/de/jeanlucmakiola/agendula/data/sync/HttpClientConstructionTest.kt @@ -0,0 +1,41 @@ +package de.jeanlucmakiola.agendula.data.sync + +import com.google.common.truth.Truth.assertThat +import org.junit.jupiter.api.Test +import java.io.File + +/** + * `network_security_config.xml` permits cleartext app-wide, and the per-account + * opt-in is enforced only by the clients `CalDavHttp` builds. Any HTTP client + * built elsewhere would speak plain http without asking. + */ +class HttpClientConstructionTest { + + private val forbidden = listOf( + "OkHttpClient()", + "OkHttpClient.Builder()", + "HttpURLConnection", + "HttpsURLConnection", + ".openConnection(", + ".openStream(", + ) + + @Test + fun `every HTTP client is built through CalDavHttp`() { + val roots = listOf(File("src"), File("../caldav/src/main")) + assertThat(roots.filter { it.isDirectory }).isNotEmpty() + + val offenders = roots + .filter { it.isDirectory } + .flatMap { root -> root.walk().filter { it.isFile && it.extension in setOf("kt", "java") }.toList() } + .filterNot { "/test" in it.invariantSeparatorsPath || "/androidTest" in it.invariantSeparatorsPath } + .filterNot { it.name == "CalDavHttp.kt" } + .flatMap { file -> + file.readLines().mapIndexedNotNull { index, line -> + forbidden.firstOrNull { it in line }?.let { "${file.path}:${index + 1}: $it" } + } + } + + assertThat(offenders).isEmpty() + } +} diff --git a/app/src/testFull/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModelTest.kt b/app/src/testFull/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModelTest.kt index 17803ea..622f82d 100644 --- a/app/src/testFull/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModelTest.kt +++ b/app/src/testFull/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModelTest.kt @@ -1525,18 +1525,52 @@ class AddAccountViewModelTest { } @Test - fun `a certificate already pinned is not asked about again`() = runTest(dispatcher) { + fun `a certificate trusted for one address is not carried to another`() = runTest(dispatcher) { gateway.discoveryOutcomes += CalDavDiscovery.Outcome.UntrustedCertificate(certificate) + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = null + val vm = viewModel() + vm.enterAddress("https://a.lan") + vm.onAddressSubmitted() + advanceUntilIdle() + vm.onCertificateTrusted() + advanceUntilIdle() + assertThat(vm.onBackWithin()).isTrue() + + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + vm.enterAddress("https://b.example.com") + vm.onAddressSubmitted() + advanceUntilIdle() + + assertThat(gateway.discoveries.last().trust).isEqualTo(ServerTrust.DEFAULT) + } + + @Test + fun `declining a certificate is offered without the back gesture`() = runTest(dispatcher) { gateway.discoveryOutcomes += CalDavDiscovery.Outcome.UntrustedCertificate(certificate) val vm = viewModel() vm.enterAddress("https://nas.home.arpa") vm.onAddressSubmitted() advanceUntilIdle() - vm.onCertificateTrusted() + + assertThat(vm.onBackWithin()).isTrue() + advanceUntilIdle() + assertThat((vm.state.value.step as AddAccountStep.EnterAddress).input).isEqualTo("https://nas.home.arpa") + assertThat(gateway.discoveries).hasSize(1) + } + + @Test + fun `an email whose domain will not set up TLS asks for the server`() = runTest(dispatcher) { + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.Failed(CalDavDiscovery.Outcome.Cause.TLS, "tls") + val vm = viewModel() + vm.onProviderChosen(ProviderChoice.Service(CalDavProvider.MAILBOX_ORG)) + vm.onAddressChanged("me@mailbox.org") + vm.onAddressSubmitted() advanceUntilIdle() - assertThat((vm.state.value.step as AddAccountStep.EnterAddress).error) - .isEqualTo(CalDavDiscovery.Outcome.Cause.TLS) + val step = vm.state.value.step as AddAccountStep.EnterAddress + assertThat(step.manualServer).isTrue() + assertThat(step.error).isNull() } @Test @@ -1553,6 +1587,42 @@ class AddAccountViewModelTest { assertThat(gateway.discoveries.single().trust).isEqualTo(stored) } + @Test + fun `a stored trust that loads late is not applied to an edited address`() = runTest(dispatcher) { + creator.storedTrust[9] = ServerTrust(allowCleartext = true) + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.Failed( + CalDavDiscovery.Outcome.Cause.INSECURE, + "http", + ) + val vm = viewModel() + vm.startReauthentication(9, "http://nas.local/dav/principals/me/", "me") + // Submitted before the stored trust has been read. + vm.onAddressChanged("http://other.local") + vm.onAddressSubmitted() + advanceUntilIdle() + + assertThat(gateway.discoveries.single().trust).isEqualTo(ServerTrust.DEFAULT) + assertThat(vm.state.value.step).isEqualTo(AddAccountStep.ConfirmCleartext("http://other.local")) + } + + @Test + fun `a renewed certificate replaces the one the account trusted`() = runTest(dispatcher) { + creator.storedTrust[9] = ServerTrust(pinnedCertificates = setOf("cd".repeat(32))) + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.UntrustedCertificate(certificate) + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = null + val vm = viewModel() + vm.startReauthentication(9, "https://nas.home.arpa/dav/principals/me/", "me") + advanceUntilIdle() + vm.onAddressSubmitted() + advanceUntilIdle() + vm.onCertificateTrusted() + advanceUntilIdle() + + assertThat(gateway.discoveries.last().trust.pinnedCertificates) + .containsExactly(certificate.fingerprint) + } + @Test fun `starting over forgets what was trusted`() = runTest(dispatcher) { gateway.discoveryOutcomes += CalDavDiscovery.Outcome.UntrustedCertificate(certificate) diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt index 4eb5869..ec5e0e0 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt @@ -157,6 +157,11 @@ class CalDavDiscovery( * A 401 stops the walk immediately: it means we found a DAV server and simply * have no credentials for it, and continuing down the ladder would replace a * precise "sign in" with a vague "nothing found". + * + * An untrusted certificate does not: a lower-priority SRV target may answer + * with one the phone trusts, so it is only offered once the ladder is spent. + * On a guessed host — the email's own domain, with no SRV record — it is not + * offered at all, since that host is most likely the domain's website. */ fun discover(input: String): Outcome { ServiceDiscovery.asBaseUrl(input)?.let { typed -> @@ -174,17 +179,31 @@ class CalDavDiscovery( } var lastFailure: Outcome = Outcome.Failed(Outcome.Cause.UNREACHABLE, "no candidate answered") + var untrusted: Outcome.UntrustedCertificate? = null + // Same host, same certificate: the remaining rungs on it would only fail again. + val refusedOrigins = mutableSetOf() for (candidate in candidates) { + if (candidate.url.originKey() in refusedOrigins) continue when (val outcome = probe(candidate.url)) { is Outcome.Found, is Outcome.NeedsAuthentication, Outcome.Unauthenticated, - is Outcome.UntrustedCertificate, -> return outcome + is Outcome.UntrustedCertificate -> { + refusedOrigins += candidate.url.originKey() + if (candidate.guessed) { + lastFailure = Outcome.Failed( + Outcome.Cause.TLS, + "untrusted certificate on guessed host ${candidate.url.host}", + ) + } else if (untrusted == null) { + untrusted = outcome + } + } else -> lastFailure = outcome } } - return lastFailure + return untrusted ?: lastFailure } internal fun probe(base: HttpUrl): Outcome { @@ -354,6 +373,8 @@ class CalDavDiscovery( untrustedCertificateIn(error)?.let { Outcome.UntrustedCertificate(ServerCertificate.of(it)) } ?: Outcome.Failed(causeOf(error), error.message ?: error.toString()) + private fun HttpUrl.originKey() = "$scheme://$host:$port" + private fun isUnauthorized(error: Throwable): Boolean = error is at.bitfire.dav4jvm.exception.UnauthorizedException diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt index c8ad5a6..9c0bdbd 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt @@ -9,7 +9,6 @@ import okhttp3.Response import okhttp3.ResponseBody.Companion.asResponseBody import okio.GzipSource import okio.buffer -import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.TimeUnit import javax.net.ssl.SSLContext import javax.net.ssl.SSLSocketFactory @@ -138,7 +137,8 @@ object CalDavHttp { ) } - private class Tls(trust: ServerTrust) { + private class Tls(pins: Set) { + private val trust = ServerTrust(pinnedCertificates = pins) val trustManager = PinningTrustManager(trust) val socketFactory: SSLSocketFactory = SSLContext.getInstance("TLS") .apply { init(null, arrayOf(trustManager), null) } @@ -147,13 +147,21 @@ object CalDavHttp { } /** - * One socket factory per distinct trust, because OkHttp only pools a + * One socket factory per distinct pin set, because OkHttp only pools a * connection between clients whose factory and verifier are the same * instances — a fresh one per call would reopen TLS for every client. + * Keyed on the pins alone (cleartext has no bearing on TLS), and bounded, + * since every abandoned setup attempt leaves a pin set behind. */ - private val tlsCache = ConcurrentHashMap() + private val tlsCache = object : LinkedHashMap, Tls>(TLS_CACHE_SIZE, 0.75f, true) { + override fun removeEldestEntry(eldest: MutableMap.MutableEntry, Tls>) = size > TLS_CACHE_SIZE + } - private fun tlsFor(trust: ServerTrust): Tls = tlsCache.getOrPut(trust) { Tls(trust) } + private fun tlsFor(trust: ServerTrust): Tls = synchronized(tlsCache) { + tlsCache.getOrPut(trust.pinnedCertificates) { Tls(trust.pinnedCertificates) } + } + + private const val TLS_CACHE_SIZE = 16 /** * The two headers that decide whether ETags are usable and whether our bytes diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt index 4ed5a22..b6e1fc3 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt @@ -51,8 +51,14 @@ object ServiceDiscovery { private const val SRV_SERVICE = "_caldavs._tcp" private const val WELL_KNOWN = "/.well-known/caldav" - /** A URL to probe, and where it came from — the "why" a failure report needs. */ - data class Candidate(val url: HttpUrl, val origin: String) + /** + * A URL to probe, and where it came from — the "why" a failure report needs. + * + * @property guessed the host is the email's domain taken on spec, with no + * SRV record naming it. Usually that is the domain's website, so its + * certificate is not one worth asking the user to trust. + */ + data class Candidate(val url: HttpUrl, val origin: String, val guessed: Boolean = false) /** * Hosts whose SRV record leads somewhere that is not a DAV server. @@ -149,16 +155,15 @@ object ServiceDiscovery { // explicit, which is the whole point of Posteo's 8443. val port = if (record.port == 443) "" else ":${record.port}" "https://$host$port" to "SRV $host$port" - }.ifEmpty { - listOf("https://$domain" to "domain as typed") } + val guessed = origins.isEmpty() - for ((origin, label) in origins) { + for ((origin, label) in origins.ifEmpty { listOf("https://$domain" to "domain as typed") }) { if (txtPath != null) { - add(candidates, origin, txtPath, "$label + TXT path=$txtPath") + add(candidates, origin, txtPath, "$label + TXT path=$txtPath", guessed) } - add(candidates, origin, WELL_KNOWN, "$label + .well-known") - add(candidates, origin, "/", "$label + root") + add(candidates, origin, WELL_KNOWN, "$label + .well-known", guessed) + add(candidates, origin, "/", "$label + root", guessed) } return candidates } @@ -181,9 +186,15 @@ object ServiceDiscovery { return url.topPrivateDomain() ?: url.host } - private fun add(into: MutableList, origin: String, path: String, label: String) { + private fun add( + into: MutableList, + origin: String, + path: String, + label: String, + guessed: Boolean = false, + ) { val url = (origin.trimEnd('/') + "/" + path.trimStart('/')).toHttpUrlOrNull() ?: return - if (into.none { it.url == url }) into += Candidate(url, label) + if (into.none { it.url == url }) into += Candidate(url, label, guessed) } /** diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServerTrustTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServerTrustTest.kt index 44b1202..9e2762f 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServerTrustTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServerTrustTest.kt @@ -12,6 +12,7 @@ import org.junit.Before import org.junit.Test import java.io.IOException import java.net.ConnectException +import java.net.InetAddress import java.net.UnknownServiceException import java.security.cert.CertificateException import javax.net.ssl.SSLContext @@ -160,4 +161,51 @@ class ServerTrustTest { assertThat(discovery.probe(server.url("/dav/"))) .isInstanceOf(CalDavDiscovery.Outcome.NeedsAuthentication::class.java) } + + @Test + fun `an untrusted SRV target does not stop the ladder before one that answers`() { + val loopback = InetAddress.getByName("127.0.0.1") + fun tlsServer(certificate: HeldCertificate) = MockWebServer().apply { + useHttps(HandshakeCertificates.Builder().heldCertificate(certificate).build().sslSocketFactory(), false) + start(loopback, 0) + } + val untrusted = tlsServer(selfSigned("127.0.0.1")) + val pinned = selfSigned("127.0.0.1") + val answering = tlsServer(pinned).apply { + enqueue(options()) + enqueue(MockResponse().setResponseCode(401)) + } + val dns = object : DnsResolver { + override fun lookup(name: String) = DnsRecords( + srv = listOf( + SrvRecord(priority = 0, weight = 0, port = untrusted.port, target = "127.0.0.1."), + SrvRecord(priority = 10, weight = 0, port = answering.port, target = "127.0.0.1."), + ), + txt = emptyList(), + ) + } + val trust = ServerTrust.DEFAULT.withCertificate(fingerprintOf(pinned.certificate)) + + try { + val outcome = CalDavDiscovery(CalDavHttp.anonymous("test", trust), dns).discover("me@127.0.0.1") + assertThat(outcome).isInstanceOf(CalDavDiscovery.Outcome.NeedsAuthentication::class.java) + } finally { + untrusted.shutdown() + answering.shutdown() + } + } + + @Test + fun `the email's own domain is a guess, and an SRV target is not`() { + assertThat(ServiceDiscovery.candidatesFor("me@example.com").all { it.guessed }).isTrue() + + val dns = object : DnsResolver { + override fun lookup(name: String) = DnsRecords( + srv = listOf(SrvRecord(priority = 0, weight = 0, port = 443, target = "dav.example.com.")), + txt = emptyList(), + ) + } + assertThat(ServiceDiscovery.candidatesFor("me@example.com", dns).none { it.guessed }).isTrue() + assertThat(ServiceDiscovery.candidatesFor("https://example.com").none { it.guessed }).isTrue() + } }