diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CalDavGateway.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CalDavGateway.kt index eccb87c..0e18101 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CalDavGateway.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CalDavGateway.kt @@ -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 } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt index ef62743..f83702a 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt @@ -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 = 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? { + 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 = diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt index 47bbef1..135b228 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt @@ -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() + var revokeSucceeds = true + + override suspend fun revokeIssuedAppPassword( + credentials: CalDavGateway.Credentials, + ): Boolean { + revoked += credentials + return revokeSucceeds + } } private class FakeCreator : AccountCreator { diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt index aad10ae..4fd00a7 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/AppPassword.kt @@ -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()) diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt index 1115006..74d8254 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/AppPasswordTest.kt @@ -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))