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.

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.
      *

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")
     }
 }

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<ArrayList<String>>(certificatesName).orEmpty().toSet(),
+        allowCleartext = handle[cleartextName] ?: false,
+    )
+
+    fun write(handle: SavedStateHandle, trust: ServerTrust) {
+        handle[certificatesName] = ArrayList(trust.pinnedCertificates)
+        handle[cleartextName] = trust.allowCleartext
+    }
+}

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")
 }

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),

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),

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<SyncNotice> = 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.

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

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<ArrayList<String>>(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<Unit>, val back: AddAccountStep)
+
+    private var trustPrompt: TrustPrompt? = null
 
     /** Which hosts asked for credentials, for the cross-domain diagnostic. */
     private var hostsNeedingAuth: List<String>
@@ -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,
         )
 
         /**

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

app/src/main/res/values/strings.xml

@@ -832,6 +832,7 @@
     <string name="add_account_certificate_issuer">Issued by</string>
     <string name="add_account_certificate_expires">Valid until</string>
     <string name="add_account_certificate_trust">Trust this certificate</string>
+    <string name="add_account_certificate_decline">Don\u2019t trust it</string>
 
     <string name="add_account_lists_title">Which lists should sync?</string>
     <string name="add_account_lists_read_only">Read-only</string>

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()
+    }
+}

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)

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<String>()
         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
 

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<String>) {
+        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<ServerTrust, Tls>()
+    private val tlsCache = object : LinkedHashMap<Set<String>, Tls>(TLS_CACHE_SIZE, 0.75f, true) {
+        override fun removeEldestEntry(eldest: MutableMap.MutableEntry<Set<String>, 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

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<Candidate>, origin: String, path: String, label: String) {
+    private fun add(
+        into: MutableList<Candidate>,
+        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)
     }
 
     /**

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()
+    }
 }