sync: hand back an app password the flow is not going to use

Nextcloud returns a minted password exactly once. Every path that left the browser flow without saving an account dropped it: discovery failing after approval, an account whose lists we cannot use, start over, the back arrow, switching to a typed password, or leaving Settings altogether. It stays valid on the server for ever, and every attempt is named "Agendula (Android)" -- so a user retrying against a misconfigured server ends up with six identical entries and no way to tell which one their working account uses. They prune nothing, or prune the wrong one. One owned field and one sink rather than a revoke per path: ten paths today, and the eleventh would be forgotten. Ownership passes to the account on Created and is released nowhere else without revoking. The sink runs on the application scope, not viewModelScope -- androidx closes that before onCleared, so a launch there never runs its body. Revocation goes through a new revokeAt, which takes the OCS root directly. ocsRootFor is principal-shaped and falls back to the bare origin, so sending a login flow's server base through it would collapse a subpath install's /nextcloud/ to / and DELETE a path that 404s -- the silent no-op that function exists to prevent. Also stops reporting a rejected credential as an empty account: a 401 after approval is either a home set outside the domain the credential is scoped to, which retrying only mints another password for, or the server having a moment. The registrable-domain check that decides this is now shared with crossDomainHint, which had been computing "com" for example.com and so never firing.
2026-09-07 22:59:39 +02:00
parent c2166b944f
commit 5dfbe0fffc
5 changed files with 293 additions and 9 deletions
@@ -39,6 +39,17 @@ interface CalDavGateway {
     */
    suspend fun revokeAppPassword(credentials: Credentials): Boolean

+    /**
+     * The same, for a password the login flow just minted.
+     *
+     * ⚠️ Separate because [Credentials.origin] means something different here:
+     * the *server root* the flow reported, not a principal URL. Sending it
+     * through [revokeAppPassword] would derive the OCS root as though it were a
+     * principal, and a subpath install's `https://host/nextcloud/` would collapse
+     * to `https://host/` — a DELETE that 404s on every one of them.
+     */
+    suspend fun revokeIssuedAppPassword(credentials: Credentials): Boolean
+
    /** Credentials, and the origin whose registrable domain they are scoped to. */
    data class Credentials(val username: String, val password: String, val origin: HttpUrl)
 }
@@ -87,5 +98,14 @@ class OkHttpCalDavGateway @Inject constructor(
        AppPassword.revoke(client, credentials.origin)
    }

+    override suspend fun revokeIssuedAppPassword(
+        credentials: CalDavGateway.Credentials,
+    ): Boolean = withContext(io) {
+        val client = CalDavHttp.authenticated(
+            userAgent, credentials.username, credentials.password, credentials.origin,
+        )
+        AppPassword.revokeAt(client, credentials.origin)
+    }
+
    private fun now() = System.currentTimeMillis() / 1000
 }
@@ -11,6 +11,8 @@ import de.jeanlucmakiola.caldav.NextcloudLoginFlow
 import de.jeanlucmakiola.caldav.ServerQuirk
 import de.jeanlucmakiola.caldav.ServiceDiscovery
 import de.jeanlucmakiola.caldav.TaskCollection
+import de.jeanlucmakiola.agendula.data.di.ApplicationScope
+import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Job
 import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.MutableStateFlow
@@ -19,6 +21,7 @@ import kotlinx.coroutines.flow.asStateFlow
 import kotlinx.coroutines.flow.update
 import kotlinx.coroutines.launch
 import okhttp3.HttpUrl
+import okhttp3.HttpUrl.Companion.toHttpUrlOrNull
 import javax.inject.Inject

 /** Where the user is in adding an account. */
@@ -86,6 +89,10 @@ data class AddAccountUiState(
 class AddAccountViewModel @Inject constructor(
    private val repository: AccountCreator,
    private val gateway: CalDavGateway,
+    // ⚠️ Not viewModelScope. Handing the password back has to survive the
+    // ViewModel that minted it, and androidx closes viewModelScope *before*
+    // onCleared runs, so a launch there never executes its body.
+    @ApplicationScope private val appScope: CoroutineScope,
 ) : ViewModel() {

    private val _state = MutableStateFlow(AddAccountUiState())
@@ -101,6 +108,17 @@ class AddAccountViewModel @Inject constructor(
    /** Which hosts asked for credentials, for the cross-domain diagnostic. */
    private var hostsNeedingAuth: List<String> = emptyList()

+    /**
+     * A password the browser flow minted and nothing owns yet.
+     *
+     * Nextcloud returns it exactly once, so if we walk away without either
+     * saving or revoking it, it stays valid in the user's device list for ever —
+     * under the same name as every other attempt, so they cannot tell which one
+     * their working account uses and dare not prune any. Cleared without
+     * revoking only when [AccountCreator] takes ownership.
+     */
+    private var minted: CalDavGateway.Credentials? = null
+
    fun onServerInputChanged(value: String) = _state.update {
        it.copy(step = AddAccountStep.EnterServer(value), quirk = ServerQuirk.forInput(value))
    }
@@ -227,6 +245,12 @@ class AddAccountViewModel @Inject constructor(
                        username = result.credentials.loginName
                        appPassword = result.credentials.appPassword
                        serverRoot = result.credentials.server
+                        // Ours to hand back until an account takes it on.
+                        minted = CalDavGateway.Credentials(
+                            username,
+                            appPassword,
+                            result.credentials.server,
+                        )
                        // The server contradicted itself about where it lives.
                        // We recovered, but the setting behind it is the user's
                        // to fix, so say so rather than silently coercing.
@@ -254,7 +278,28 @@ class AddAccountViewModel @Inject constructor(
                            // no task lists, which is both wrong and unactionable.
                            is CalDavDiscovery.Outcome.Failed -> backToServer(outcome.cause)
                            is CalDavDiscovery.Outcome.NotCalDav -> backToServer(outcome.cause)
-                            else -> backToServer(CalDavDiscovery.Outcome.Cause.NO_CALENDARS)
+                            // ⚠️ These two carry no Cause, so they used to land
+                            // on "signed in, but no task lists" — said of a
+                            // credential the server had just rejected. A 401 here
+                            // is either a home set outside the domain the
+                            // credential is scoped to, which retrying cannot fix
+                            // and which only mints a second password, or the
+                            // server having a moment.
+                            is CalDavDiscovery.Outcome.NeedsAuthentication -> {
+                                hostsNeedingAuth = outcome.hosts
+                                val stranger = outsideCredentialScope(outcome.hosts)
+                                if (stranger != null) {
+                                    fatal(
+                                        "This server keeps some task lists on $stranger, " +
+                                            "which Agendula cannot sign in to yet.",
+                                    )
+                                } else {
+                                    backToServer(CalDavDiscovery.Outcome.Cause.SERVER_ERROR)
+                                }
+                            }
+
+                            CalDavDiscovery.Outcome.Unauthenticated ->
+                                backToServer(CalDavDiscovery.Outcome.Cause.SERVER_ERROR)
                        }
                        return@launch
                    }
@@ -278,6 +323,9 @@ class AddAccountViewModel @Inject constructor(

    /** The user says they finished in the browser but nothing arrived. */
    fun onBrowserCancelled() {
+        // The one exit that never reaches onStartOver: if approval already
+        // landed, that password is about to be replaced by a typed one.
+        discardMintedPassword()
        pollJob?.cancel()
        _state.update {
            it.copy(step = AddAccountStep.EnterCredentials(error = null), openInBrowser = null)
@@ -322,8 +370,13 @@ class AddAccountViewModel @Inject constructor(
                )
            }
            when (outcome) {
-                is AccountRepository.Outcome.Created ->
+                is AccountRepository.Outcome.Created -> {
+                    // Ownership passes to the account, which revokes on removal.
+                    // Cleared before the state update: an exception there must not
+                    // leave a saved account's own credential queued for revoking.
+                    minted = null
                    _state.update { it.copy(step = AddAccountStep.Done) }
+                }

                AccountRepository.Outcome.AlreadyExists ->
                    fatal("That account is already set up.")
@@ -344,6 +397,7 @@ class AddAccountViewModel @Inject constructor(
     * account's username and app password.
     */
    fun onStartOver() {
+        discardMintedPassword()
        pollJob?.cancel()
        pollJob = null
        found = null
@@ -356,9 +410,28 @@ class AddAccountViewModel @Inject constructor(
    }

    override fun onCleared() {
+        abandonMintedPassword()
        pollJob?.cancel()
    }

+    /** [onCleared] is protected, and the abandonment is worth testing. */
+    internal fun abandonMintedPassword() = discardMintedPassword()
+
+    /**
+     * Hand back a password nothing is going to use.
+     *
+     * Fire and forget: the user is already leaving, the call is best-effort by
+     * the module's own contract, and it carries its own 5s budget. A failure is
+     * not reported — telling someone we could not clean up a credential they
+     * never knew existed, while they are escaping a failure, is worse than the
+     * row it leaves behind.
+     */
+    private fun discardMintedPassword() {
+        val credentials = minted ?: return
+        minted = null
+        appScope.launch { gateway.revokeIssuedAppPassword(credentials) }
+    }
+
    // ------------------------------------------------------------- internals

    private fun onDiscovered(outcome: CalDavDiscovery.Outcome.Found) {
@@ -430,13 +503,26 @@ class AddAccountViewModel @Inject constructor(
     * legal (RFC 4791 §6.2.1) but unreachable for us: the credential is scoped to
     * one domain. Better to name it than to leave "wrong password" standing.
     */
-    private fun crossDomainHint(): String? {
-        val root = serverRoot?.host ?: return null
-        val rootDomain = root.substringAfter('.', root)
-        val outside = hostsNeedingAuth.filterNot { it.endsWith(rootDomain) }
-        return outside.firstOrNull()?.let {
+    private fun crossDomainHint(): String? =
+        outsideCredentialScope(hostsNeedingAuth)?.let {
            "This server keeps some task lists on $it, which Agendula cannot sign in to yet."
        }
+
+    /**
+     * The first host the credential will never be offered to, if any.
+     *
+     * ⚠️ The same boundary `CalDavHttp` scopes the credential by, derived the
+     * same way. A `substringAfter('.')` split computes "com" for
+     * `example.com` — so every host ending in `com` reads as in-domain, the
+     * diagnostic never fires, and `notexample.com` reads as in-domain too.
+     */
+    private fun outsideCredentialScope(hosts: List<String>): String? {
+        val root = serverRoot ?: return null
+        val scope = root.topPrivateDomain() ?: root.host
+        return hosts.firstOrNull { host ->
+            val candidate = "https://$host".toHttpUrlOrNull() ?: return@firstOrNull true
+            !(candidate.topPrivateDomain() ?: candidate.host).equals(scope, ignoreCase = true)
+        }
    }

    private fun accountName(): String =
@@ -7,12 +7,14 @@ import de.jeanlucmakiola.agendula.data.sync.CalDavGateway
 import de.jeanlucmakiola.caldav.CalDavDiscovery
 import de.jeanlucmakiola.caldav.NextcloudLoginFlow
 import de.jeanlucmakiola.caldav.TaskCollection
+import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.ExperimentalCoroutinesApi
 import kotlinx.coroutines.test.StandardTestDispatcher
 import kotlinx.coroutines.test.advanceUntilIdle
 import kotlinx.coroutines.test.runCurrent
 import kotlinx.coroutines.test.resetMain
+import kotlinx.coroutines.test.TestScope
 import kotlinx.coroutines.test.runTest
 import kotlinx.coroutines.test.setMain
 import okhttp3.HttpUrl
@@ -40,7 +42,15 @@ class AddAccountViewModelTest {

    @AfterEach fun tearDown() = Dispatchers.resetMain()

-    private fun viewModel() = AddAccountViewModel(creator, gateway)
+    /**
+     * ⚠️ Not `backgroundScope`: `advanceUntilIdle` does not run its work, so a
+     * revocation launched there would silently never happen and every assertion
+     * about it would pass for the wrong reason. This scope shares the scheduler
+     * as ordinary work.
+     */
+    private val appScope = CoroutineScope(dispatcher)
+
+    private fun viewModel() = AddAccountViewModel(creator, gateway, appScope)

    @Nested
    inner class TheAddressStep {
@@ -249,6 +259,134 @@ class AddAccountViewModelTest {
        }
    }

+    @Nested
+    inner class AbandoningTheBrowserFlow {
+
+        private fun TestScope.approveThen(outcome: CalDavDiscovery.Outcome): AddAccountViewModel {
+            gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(
+                listOf("cloud.example.com"),
+            )
+            gateway.loginFlow = flow()
+            gateway.pollResults += NextcloudLoginFlow.PollResult.Approved(
+                NextcloudLoginFlow.Credentials(
+                    server = "https://dav.example.com/".toHttpUrl(),
+                    loginName = "me",
+                    appPassword = "app-pw",
+                ),
+            )
+            gateway.discoveryOutcomes += outcome
+            val vm = viewModel()
+            vm.onServerInputChanged("https://cloud.example.com/")
+            vm.onServerSubmitted()
+            advanceUntilIdle()
+            return vm
+        }
+
+        @Test
+        fun `a password nobody will use is handed back`() = runTest(dispatcher) {
+            val vm = approveThen(
+                CalDavDiscovery.Outcome.Failed(
+                    CalDavDiscovery.Outcome.Cause.UNREACHABLE,
+                    "no route to host",
+                ),
+            )
+
+            vm.onStartOver()
+            advanceUntilIdle()
+
+            // Nextcloud hands it over exactly once. Walking away leaves it valid
+            // for ever, under the same name as every other attempt — so the user
+            // cannot tell which entry their working account uses.
+            val handedBack = gateway.revoked.single()
+            assertThat(handedBack.password).isEqualTo("app-pw")
+            // The server that issued it, not the one the user typed.
+            assertThat(handedBack.origin.toString()).isEqualTo("https://dav.example.com/")
+        }
+
+        @Test
+        fun `an account with no usable lists hands its password back`() = runTest(dispatcher) {
+            val vm = approveThen(found())
+
+            // fatal() offers only "start over", so that is the whole exit.
+            vm.onStartOver()
+            advanceUntilIdle()
+
+            assertThat(gateway.revoked).hasSize(1)
+        }
+
+        @Test
+        fun `leaving the screen hands the password back`() = runTest(dispatcher) {
+            val vm = approveThen(
+                CalDavDiscovery.Outcome.Failed(
+                    CalDavDiscovery.Outcome.Cause.UNREACHABLE,
+                    "no route to host",
+                ),
+            )
+
+            vm.abandonMintedPassword()
+            advanceUntilIdle()
+
+            assertThat(gateway.revoked).hasSize(1)
+        }
+
+        @Test
+        fun `a saved account keeps its password`() = runTest(dispatcher) {
+            val vm = approveThen(found(collection("Tasks")))
+            vm.onSave()
+            advanceUntilIdle()
+
+            // What the screen does once it is Done.
+            vm.onStartOver()
+            advanceUntilIdle()
+
+            // ⚠️ The one way this fix can do real harm: revoking the credential
+            // of the account just created. It would sync once and then 401 for
+            // ever, long after anyone connects it to adding the account.
+            assertThat(gateway.revoked).isEmpty()
+        }
+
+        @Test
+        fun `a revoke that fails is never surfaced`() = runTest(dispatcher) {
+            gateway.revokeSucceeds = false
+            val vm = approveThen(
+                CalDavDiscovery.Outcome.Failed(
+                    CalDavDiscovery.Outcome.Cause.UNREACHABLE,
+                    "no route to host",
+                ),
+            )
+
+            vm.onStartOver()
+            advanceUntilIdle()
+
+            assertThat(vm.state.value.fatal).isNull()
+            assertThat(vm.state.value.step).isInstanceOf(AddAccountStep.EnterServer::class.java)
+        }
+
+        @Test
+        fun `a rejected password is not reported as an empty account`() = runTest(dispatcher) {
+            val vm = approveThen(
+                CalDavDiscovery.Outcome.NeedsAuthentication(listOf("cloud.example.com")),
+            )
+
+            // "Signed in, but no task lists" said of a credential the server had
+            // just refused. Same registrable domain, so retrying may help.
+            val step = vm.state.value.step as AddAccountStep.EnterServer
+            assertThat(step.error).isEqualTo(CalDavDiscovery.Outcome.Cause.SERVER_ERROR)
+        }
+
+        @Test
+        fun `a home set outside the credential scope is called what it is`() =
+            runTest(dispatcher) {
+                val vm = approveThen(
+                    CalDavDiscovery.Outcome.NeedsAuthentication(listOf("dav.elsewhere.org")),
+                )
+
+                // Retrying cannot reach it, and each attempt mints another
+                // password — so this is fatal rather than back-to-the-address.
+                assertThat(vm.state.value.fatal).contains("dav.elsewhere.org")
+            }
+    }
+
    @Nested
    inner class ChoosingLists {

@@ -418,6 +556,16 @@ class AddAccountViewModelTest {
        override suspend fun revokeAppPassword(
            credentials: CalDavGateway.Credentials,
        ): Boolean = true
+
+        val revoked = mutableListOf<CalDavGateway.Credentials>()
+        var revokeSucceeds = true
+
+        override suspend fun revokeIssuedAppPassword(
+            credentials: CalDavGateway.Credentials,
+        ): Boolean {
+            revoked += credentials
+            return revokeSucceeds
+        }
    }

    private class FakeCreator : AccountCreator {
@@ -81,8 +81,24 @@ object AppPassword {
        httpClient: OkHttpClient,
        principal: HttpUrl,
        timeout: Duration = REVOCATION_TIMEOUT,
+    ): Boolean = revokeAt(httpClient, ocsRootFor(principal), timeout)
+
+    /**
+     * The same revocation, for a caller that already holds the server root.
+     *
+     * ⚠️ Do not route such a caller through [revoke]. [ocsRootFor] is
+     * *principal*-shaped: it looks for `remote.php` and falls back to the bare
+     * origin when there is none. A login flow hands back a server base, so a
+     * subpath install's `https://host/nextcloud/` would collapse to
+     * `https://host/` and the DELETE would 404 on every one of them — attempted,
+     * and doing nothing, which is the failure [ocsRootFor] exists to prevent.
+     */
+    fun revokeAt(
+        httpClient: OkHttpClient,
+        ocsRoot: HttpUrl,
+        timeout: Duration = REVOCATION_TIMEOUT,
    ): Boolean = try {
-        val url = ocsRootFor(principal).newBuilder().addPathSegments(PATH).build()
+        val url = ocsRoot.newBuilder().addPathSegments(PATH).build()
        httpClient.newBuilder()
            // Shares the pool and dispatcher, so this costs nothing.
            .callTimeout(timeout.toJavaDuration())
@@ -42,6 +42,20 @@ class AppPasswordTest {
            .isEqualTo("https://baikal.example.com/")
    }

+    @Test fun `a server root is used verbatim, not treated as a principal`() {
+        server.enqueue(MockResponse().setResponseCode(200))
+
+        val revoked = AppPassword.revokeAt(httpClient, server.url("/nextcloud/"))
+
+        // ⚠️ ocsRootFor looks for remote.php and falls back to the bare origin.
+        // A login flow hands back a server base, so routing one through revoke()
+        // would collapse /nextcloud/ to / and DELETE a path that 404s on every
+        // subpath install — attempted, and doing nothing.
+        assertThat(revoked).isTrue()
+        assertThat(server.takeRequest().path)
+            .isEqualTo("/nextcloud/ocs/v2.php/core/apppassword")
+    }
+
    @Test fun `a server that never answers does not hold the removal`() {
        // Accepts the connection and says nothing — the off-VPN homelab shape.
        server.enqueue(MockResponse().setSocketPolicy(SocketPolicy.NO_RESPONSE))