sync: hand the password back on the retry path too
5dfbe0f discarded on start over, the back arrow and leaving the screen —
but after a post-approval failure the screen shows the address field
with an error and a *Continue* button, and there is no start over to
press. So the retry the user actually makes ran a second login flow and
overwrote the first password without handing it back, which is the leak
the commit was written to close. Its test called onStartOver, an
affordance that state never offers, so it passed against a path nobody
can take.
onServerSubmitted now discards, since it begins a fresh attempt.
Three smaller ones from the same review. The revoke runs without a
catch on a scope that has no exception handler, so anything escaping
OkHttp outside AppPassword's own try would take the app down for a
courtesy call whose failures are deliberately silent. An unparseable
host counted as out-of-scope, and since a stranger is now fatal, an
IPv6 literal — which OkHttp hands back unbracketed — would strand a
homelab at [::1]; it brackets first and reads unparseable as in-scope,
so the diagnostic fails quiet rather than into a dead end. And the
one-shot 200 is recorded uncancellably: a cancellation delivered
between the server deleting the flow row and our writing the credential
down spends it with nothing left holding it.This commit is contained in:
@@ -13,6 +13,8 @@ import de.jeanlucmakiola.caldav.ServiceDiscovery
|
||||
import de.jeanlucmakiola.caldav.TaskCollection
|
||||
import de.jeanlucmakiola.agendula.data.di.ApplicationScope
|
||||
import kotlinx.coroutines.CoroutineScope
|
||||
import kotlinx.coroutines.NonCancellable
|
||||
import kotlinx.coroutines.withContext
|
||||
import kotlinx.coroutines.Job
|
||||
import kotlinx.coroutines.delay
|
||||
import kotlinx.coroutines.flow.MutableStateFlow
|
||||
@@ -131,6 +133,11 @@ class AddAccountViewModel @Inject constructor(
|
||||
fun onServerSubmitted() {
|
||||
val input = (_state.value.step as? AddAccountStep.EnterServer)?.input?.trim().orEmpty()
|
||||
if (input.isEmpty()) return
|
||||
// ⚠️ The retry path, and the one that actually leaks. A post-approval
|
||||
// failure sends the user back here with an error, and the screen offers
|
||||
// *Continue*, not start over — so a second attempt would mint a second
|
||||
// password on top of the first without this.
|
||||
discardMintedPassword()
|
||||
typedInput = input
|
||||
|
||||
val quirk = ServerQuirk.forInput(input)
|
||||
@@ -242,15 +249,23 @@ class AddAccountViewModel @Inject constructor(
|
||||
delay(POLL_INTERVAL_MILLIS)
|
||||
when (val result = gateway.pollLoginFlow(flow)) {
|
||||
is NextcloudLoginFlow.PollResult.Approved -> {
|
||||
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,
|
||||
)
|
||||
// ⚠️ Uncancellable. The 200 that carried this password is
|
||||
// the only one there will ever be — the server deletes the
|
||||
// flow row before answering — so a cancellation delivered
|
||||
// between that answer and the assignment below spends the
|
||||
// credential with nothing left holding it. Recording it is
|
||||
// what makes handing it back possible.
|
||||
withContext(NonCancellable) {
|
||||
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.
|
||||
@@ -371,10 +386,17 @@ class AddAccountViewModel @Inject constructor(
|
||||
}
|
||||
when (outcome) {
|
||||
is AccountRepository.Outcome.Created -> {
|
||||
// Ownership passes to the account, which revokes on removal.
|
||||
// Ownership passes to the account, which revokes on removal —
|
||||
// but only if this is the password it was saved with. Every
|
||||
// route from a minted password to a typed one already
|
||||
// discards on the way, so the else branch should be dead;
|
||||
// it is here because the failure it guards is silent and
|
||||
// delayed. The account syncs once and then 401s for ever,
|
||||
// long after anyone would connect it to adding it.
|
||||
//
|
||||
// Cleared before the state update: an exception there must not
|
||||
// leave a saved account's own credential queued for revoking.
|
||||
minted = null
|
||||
if (minted?.password == appPassword) minted = null else discardMintedPassword()
|
||||
_state.update { it.copy(step = AddAccountStep.Done) }
|
||||
}
|
||||
|
||||
@@ -429,7 +451,9 @@ class AddAccountViewModel @Inject constructor(
|
||||
private fun discardMintedPassword() {
|
||||
val credentials = minted ?: return
|
||||
minted = null
|
||||
appScope.launch { gateway.revokeIssuedAppPassword(credentials) }
|
||||
// The scope has no exception handler, so anything escaping here takes the
|
||||
// app down — for a courtesy call whose failures are deliberately silent.
|
||||
appScope.launch { runCatching { gateway.revokeIssuedAppPassword(credentials) } }
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------- internals
|
||||
@@ -520,7 +544,12 @@ class AddAccountViewModel @Inject constructor(
|
||||
val root = serverRoot ?: return null
|
||||
val scope = root.topPrivateDomain() ?: root.host
|
||||
return hosts.firstOrNull { host ->
|
||||
val candidate = "https://$host".toHttpUrlOrNull() ?: return@firstOrNull true
|
||||
// OkHttp hands back IPv6 literals unbracketed, which will not parse
|
||||
// again. Unparseable reads as in-scope: the cost of a diagnostic that
|
||||
// stays quiet is nothing, and the caller now treats out-of-scope as
|
||||
// fatal — so failing the other way strands a homelab on [::1].
|
||||
val literal = if (':' in host) "[$host]" else host
|
||||
val candidate = "https://$literal".toHttpUrlOrNull() ?: return@firstOrNull false
|
||||
!(candidate.topPrivateDomain() ?: candidate.host).equals(scope, ignoreCase = true)
|
||||
}
|
||||
}
|
||||
|
||||
+28
-1
@@ -291,7 +291,15 @@ class AddAccountViewModelTest {
|
||||
),
|
||||
)
|
||||
|
||||
vm.onStartOver()
|
||||
// ⚠️ The path the user actually has. After a post-approval failure
|
||||
// the screen shows the address with an error and a *Continue* button
|
||||
// — there is no start-over here, so a test that used one would pass
|
||||
// against an affordance nobody can reach.
|
||||
gateway.discoveryOutcomes += CalDavDiscovery.Outcome.Failed(
|
||||
CalDavDiscovery.Outcome.Cause.UNREACHABLE,
|
||||
"no route to host",
|
||||
)
|
||||
vm.onServerSubmitted()
|
||||
advanceUntilIdle()
|
||||
|
||||
// Nextcloud hands it over exactly once. Walking away leaves it valid
|
||||
@@ -345,6 +353,25 @@ class AddAccountViewModelTest {
|
||||
assertThat(gateway.revoked).isEmpty()
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a typed password after an approval hands the minted one back`() =
|
||||
runTest(dispatcher) {
|
||||
val vm = approveThen(
|
||||
CalDavDiscovery.Outcome.NeedsAuthentication(listOf("cloud.example.com")),
|
||||
)
|
||||
// Retry, this time with no login flow on offer, so the user types
|
||||
// their own password instead.
|
||||
gateway.loginFlow = null
|
||||
gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(
|
||||
listOf("cloud.example.com"),
|
||||
)
|
||||
vm.onServerSubmitted()
|
||||
advanceUntilIdle()
|
||||
|
||||
// The retry itself hands back the first password.
|
||||
assertThat(gateway.revoked.map { it.password }).containsExactly("app-pw")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a revoke that fails is never surfaced`() = runTest(dispatcher) {
|
||||
gateway.revokeSucceeds = false
|
||||
|
||||
Reference in New Issue
Block a user