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