From ead84c31959efc8a2ace0dd7aec21eda62195c20 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Wed, 9 Sep 2026 11:39:21 +0200 Subject: [PATCH] sync: the login flow is written down before the browser gets it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flow's own doc says to persist it before launching the browser, because the flow outlives our process — and nothing did. The browser is a separate task, so dying while the user approves is ordinary rather than exotic, and it stranded a one-shot app password that nothing could then collect or revoke: the poll token was the only way back to it and it lived in a ViewModel field. The token goes to the sync-state store, not the Keystore: it authorises one poll of one flow the user is in the middle of approving, and it is worthless past the twenty-minute window. It is cleared the moment the flow stops mattering — spent, expired, cancelled, started over. A genuine app open reclaims what a dead process left: poll once, and if the user did approve, hand the password straight back. Revoked rather than used, because the address they typed, the collections they ticked and the account name went with the process — what is left is a live credential in their device list under the same name as every other attempt, which is exactly the one they cannot tell apart and so dare not prune. Once per process, so a rotation cannot consume the one-shot 200 a live wizard is waiting for. The window after approval, where the password itself is only in memory, stays open — that needs the wizard's own state to survive, which is the same work as the wizard restructure. --- .../jeanlucmakiola/agendula/MainActivity.kt | 6 + .../agendula/data/di/DataModule.kt | 6 + .../data/sync/PendingLoginFlowStore.kt | 144 ++++++++++++++++++ .../ui/accounts/AddAccountViewModel.kt | 21 ++- .../ui/accounts/AddAccountViewModelTest.kt | 84 +++++++++- 5 files changed, 259 insertions(+), 2 deletions(-) create mode 100644 app/src/main/java/de/jeanlucmakiola/agendula/data/sync/PendingLoginFlowStore.kt diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/MainActivity.kt b/app/src/main/java/de/jeanlucmakiola/agendula/MainActivity.kt index c8c044e..8406e5f 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/MainActivity.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/MainActivity.kt @@ -21,6 +21,7 @@ import de.jeanlucmakiola.agendula.data.prefs.ThemeMode import de.jeanlucmakiola.agendula.ui.RootScreen import de.jeanlucmakiola.agendula.ui.crash.CrashReportActivity import de.jeanlucmakiola.agendula.data.sync.AccountRepository +import de.jeanlucmakiola.agendula.data.sync.PendingLoginFlowStore import de.jeanlucmakiola.agendula.data.sync.SyncTrigger import de.jeanlucmakiola.agendula.ui.settings.SettingsViewModel import de.jeanlucmakiola.agendula.ui.theme.AgendulaTheme @@ -44,6 +45,8 @@ class MainActivity : ComponentActivity() { @Inject lateinit var syncTrigger: SyncTrigger + @Inject lateinit var pendingLoginFlows: PendingLoginFlowStore + // A captured crash report awaiting the user's decision, surfaced as a dialog // over the app on the next launch (the single-crash path). A startup // crash-loop is handled out of band, before setContent — see below. @@ -80,6 +83,9 @@ class MainActivity : ComponentActivity() { runCatching { accounts.rescheduleAll() accounts.syncable().forEach { syncTrigger.enqueue(it.displayName) } + // A login flow the previous process died in the middle of. + // Its password, if the user approved, exists nowhere else. + pendingLoginFlows.reclaim() } } } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/di/DataModule.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/di/DataModule.kt index a207a0b..f6b5265 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/di/DataModule.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/di/DataModule.kt @@ -15,6 +15,8 @@ import dagger.hilt.components.SingletonComponent 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.PendingLoginFlowStore import de.jeanlucmakiola.agendula.data.sync.OkHttpCalDavGateway import de.jeanlucmakiola.agendula.data.tasks.AndroidProviderEnvironment import de.jeanlucmakiola.agendula.data.tasks.AndroidTasksDataSource @@ -78,6 +80,10 @@ abstract class DataBindModule { @Binds @Singleton abstract fun bindAccountCreator(impl: AccountRepository): AccountCreator + + @Binds + @Singleton + abstract fun bindLoginFlowRecord(impl: PendingLoginFlowStore): LoginFlowRecord } @Module diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/PendingLoginFlowStore.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/PendingLoginFlowStore.kt new file mode 100644 index 0000000..e63bdb1 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/PendingLoginFlowStore.kt @@ -0,0 +1,144 @@ +package de.jeanlucmakiola.agendula.data.sync + +import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.edit +import androidx.datastore.preferences.core.longPreferencesKey +import androidx.datastore.preferences.core.stringPreferencesKey +import de.jeanlucmakiola.agendula.data.di.SyncStateDataStore +import de.jeanlucmakiola.caldav.NextcloudLoginFlow +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Where a started login flow is written down, so it can outlive this process. + * + * A seam for the same reason [AccountCreator] and [CalDavGateway] are: the flow + * that decides *when* a one-shot password stops being ours has to be testable + * without a DataStore. + */ +interface LoginFlowRecord { + + /** Called **before** the browser is handed the URL. */ + suspend fun remember(flow: NextcloudLoginFlow.Flow) + + /** The flow is over, however it ended. */ + suspend fun forget() +} + +/** + * The Nextcloud login flow that is currently out at a browser. + * + * ⚠️ [NextcloudLoginFlow.Flow]'s own doc says to persist it **before** launching + * the browser, because the flow outlives our process — and it did not. The + * browser is a separate task, so process death while the user is approving is + * ordinary rather than exotic, and it stranded a one-shot app password that + * nothing could then collect *or* revoke: the flow's poll token was the only way + * back to it, and it lived in a ViewModel field. + * + * The token is not a credential. It authorises exactly one poll of one flow the + * user is in the middle of approving, and it is useless past the twenty-minute + * window — so it belongs in the sync-state store rather than the Keystore. + * + * ⚠️ What this does **not** cover is the window *after* approval, where the + * password itself lives only in memory. That needs the wizard's own state to + * survive, which is a different piece of work. + */ +@Singleton +class PendingLoginFlowStore @Inject constructor( + @SyncStateDataStore private val dataStore: DataStore, + private val gateway: CalDavGateway, +) : LoginFlowRecord { + + private val lock = Mutex() + private var reclaimed = false + + /** Records [flow] so a process that dies mid-approval can still finish with it. */ + override suspend fun remember(flow: NextcloudLoginFlow.Flow) { + dataStore.edit { prefs -> + prefs[LOGIN_URL] = flow.loginUrl.toString() + prefs[POLL_ENDPOINT] = flow.pollEndpoint.toString() + prefs[POLL_TOKEN] = flow.pollToken + prefs[DEADLINE] = flow.deadlineEpochSeconds + } + } + + /** The flow is finished, one way or another. */ + override suspend fun forget() { + dataStore.edit { prefs -> + prefs.remove(LOGIN_URL) + prefs.remove(POLL_ENDPOINT) + prefs.remove(POLL_TOKEN) + prefs.remove(DEADLINE) + } + } + + /** + * Collects and hands back a password nobody is left to own. + * + * Revoked rather than used: the address the user typed, the collections they + * ticked and the account name are all gone with the process, so there is + * nothing to finish. What is left is a live app password in the user's + * device list, under the same name as every other attempt — which is exactly + * what they cannot tell apart, and so dare not prune. + * + * ⚠️ **Once per process.** This activity is recreated on every rotation, + * theme switch and locale change, and a second run against a flow the *live* + * wizard is still polling would consume its one-shot 200 and revoke the + * password it was about to be handed. A flow remembered after this has run + * belongs to a wizard that is alive to finish it. + */ + suspend fun reclaim() { + lock.withLock { + if (reclaimed) return + reclaimed = true + } + val flow = pending() ?: return + when (val result = gateway.pollLoginFlow(flow)) { + is NextcloudLoginFlow.PollResult.Approved -> { + // Cleared first: a revocation that fails must not leave a token + // that would be polled again, and the 200 is already spent. + forget() + gateway.revokeIssuedAppPassword( + CalDavGateway.Credentials( + username = result.credentials.loginName, + password = result.credentials.appPassword, + origin = result.credentials.server, + ), + ) + } + + is NextcloudLoginFlow.PollResult.Expired -> forget() + + // Still inside the window, or the server had a moment. Either way the + // token is still worth something; the deadline retires it. + NextcloudLoginFlow.PollResult.Pending, + is NextcloudLoginFlow.PollResult.Failed, + -> Unit + } + } + + private suspend fun pending(): NextcloudLoginFlow.Flow? { + val prefs = dataStore.data.first() + val endpoint = prefs[POLL_ENDPOINT]?.toHttpUrlOrNull() ?: return null + val token = prefs[POLL_TOKEN] ?: return null + val deadline = prefs[DEADLINE] ?: return null + return NextcloudLoginFlow.Flow( + loginUrl = prefs[LOGIN_URL]?.toHttpUrlOrNull() ?: endpoint, + pollEndpoint = endpoint, + pollToken = token, + deadlineEpochSeconds = deadline, + ) + } + + 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") + } +} 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 0973cd2..67c89f1 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 @@ -6,6 +6,7 @@ import dagger.hilt.android.lifecycle.HiltViewModel 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.caldav.CalDavDiscovery import de.jeanlucmakiola.caldav.NextcloudLoginFlow import de.jeanlucmakiola.caldav.ServerQuirk @@ -91,6 +92,7 @@ data class AddAccountUiState( class AddAccountViewModel @Inject constructor( private val repository: AccountCreator, private val gateway: CalDavGateway, + private val pendingFlow: LoginFlowRecord, // ⚠️ 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. @@ -263,6 +265,11 @@ class AddAccountViewModel @Inject constructor( return } + // ⚠️ Written down **before** the browser is offered the URL, which is + // what the flow's own doc asks for: the browser is a separate task, so + // dying while the user approves is ordinary, and the poll token is the + // only way back to the password the server is about to mint. + pendingFlow.remember(flow) _state.update { it.copy( step = AddAccountStep.WaitingForBrowser(hostMismatch = flow.hostMismatch), @@ -290,6 +297,10 @@ class AddAccountViewModel @Inject constructor( // credential with nothing left holding it. Recording it is // what makes handing it back possible. withContext(NonCancellable) { + // Spent: the server deleted the row before answering, + // so a persisted token is now a thing that would be + // polled again for nothing. + pendingFlow.forget() username = result.credentials.loginName appPassword = result.credentials.appPassword serverRoot = result.credentials.server @@ -384,6 +395,7 @@ class AddAccountViewModel @Inject constructor( // The one exit that never reaches onStartOver: if approval already // landed, that password is about to be replaced by a typed one. discardMintedPassword() + appScope.launch { runCatching { pendingFlow.forget() } } pollJob?.cancel() _state.update { it.copy(step = AddAccountStep.EnterCredentials(error = null), openInBrowser = null) @@ -485,6 +497,7 @@ class AddAccountViewModel @Inject constructor( */ fun onStartOver() { discardMintedPassword() + appScope.launch { runCatching { pendingFlow.forget() } } pollJob?.cancel() pollJob = null found = null @@ -569,7 +582,13 @@ class AddAccountViewModel @Inject constructor( it.copy(step = AddAccountStep.EnterServer(input = typedInput, error = cause)) } - private fun browserFailed(reason: AddAccountMessage) = _state.update { + private fun browserFailed(reason: AddAccountMessage) { + // Whatever ended it, nothing is coming back through this token. + appScope.launch { runCatching { pendingFlow.forget() } } + failedInBrowser(reason) + } + + private fun failedInBrowser(reason: AddAccountMessage) = _state.update { // Keeps whatever the step was carrying. The host-mismatch note is often // the *explanation* for the failure, so dropping it removes the warning // exactly when it becomes worth reading. 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 57bc2f7..6c7affd 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 @@ -4,6 +4,7 @@ import com.google.common.truth.Truth.assertThat 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.caldav.CalDavDiscovery import de.jeanlucmakiola.caldav.NextcloudLoginFlow import de.jeanlucmakiola.caldav.TaskCollection @@ -37,6 +38,7 @@ class AddAccountViewModelTest { private val dispatcher = StandardTestDispatcher() private val gateway = FakeGateway() private val creator = FakeCreator() + private val record = FakeLoginFlowRecord() @BeforeEach fun setUp() = Dispatchers.setMain(dispatcher) @@ -50,7 +52,7 @@ class AddAccountViewModelTest { */ private val appScope = CoroutineScope(dispatcher) - private fun viewModel() = AddAccountViewModel(creator, gateway, appScope) + private fun viewModel() = AddAccountViewModel(creator, gateway, record, appScope) @Nested inner class TheAddressStep { @@ -259,6 +261,70 @@ class AddAccountViewModelTest { } } + @Nested + inner class SurvivingTheProcess { + + @Test + fun `the flow is written down before the browser is offered the URL`() = + runTest(dispatcher) { + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication( + listOf("cloud.example.com"), + ) + gateway.loginFlow = flow() + + val vm = viewModel() + vm.onServerInputChanged("https://cloud.example.com/") + vm.onServerSubmitted() + // Not advanceUntilIdle: that would run the whole twenty-minute + // poll loop out to its expiry, which forgets the flow again. + runCurrent() + + // ⚠️ The browser is a separate task, so dying while the user + // approves is ordinary — and the poll token is the only way back + // to the password the server is about to mint. + assertThat(record.remembered).isEqualTo(gateway.loginFlow) + assertThat(vm.state.value.openInBrowser).isNotNull() + } + + @Test + fun `a spent flow is not left behind to be polled again`() = runTest(dispatcher) { + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = flow() + gateway.pollResults += NextcloudLoginFlow.PollResult.Approved( + NextcloudLoginFlow.Credentials( + server = "https://cloud.example.com/".toHttpUrl(), + loginName = "me", + appPassword = "app-pw", + ), + ) + gateway.discoveryOutcomes += found(collection("Tasks")) + + val vm = viewModel() + vm.onServerInputChanged("https://cloud.example.com/") + vm.onServerSubmitted() + advanceUntilIdle() + + // The server deleted the flow row before answering, so the token now + // buys nothing and a reclaim would poll it for no reason. + assertThat(record.remembered).isNull() + assertThat(vm.state.value.step).isInstanceOf(AddAccountStep.ChooseLists::class.java) + } + + @Test + fun `a flow that ends badly is forgotten too`() = runTest(dispatcher) { + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = flow() + gateway.pollResults += NextcloudLoginFlow.PollResult.Expired("window closed") + + val vm = viewModel() + vm.onServerInputChanged("https://cloud.example.com/") + vm.onServerSubmitted() + advanceUntilIdle() + + assertThat(record.remembered).isNull() + } + } + @Nested inner class AbandoningTheBrowserFlow { @@ -697,6 +763,22 @@ class AddAccountViewModelTest { } } + /** Remembers what the flow told it, in the order it was told. */ + private class FakeLoginFlowRecord : LoginFlowRecord { + val log = mutableListOf() + var remembered: NextcloudLoginFlow.Flow? = null + + override suspend fun remember(flow: NextcloudLoginFlow.Flow) { + remembered = flow + log += "remember" + } + + override suspend fun forget() { + remembered = null + log += "forget" + } + } + private class FakeCreator : AccountCreator { val created = mutableListOf() var thrown: Throwable? = null