makiolaj/agendula
1
0
Fork 0
Code Issues Pull Requests Actions Releases 8 Activity

sync: four corrections to the add-account flow

Browse Source 
Restarting from the address step kept the previous server's credentials.
backToServer is reachable after a successful approval — a post-approval
discovery failure lands on an ordinary address step with a live Continue
button — and only onStartOver cleared the username and password. So:
approve on A, discovery fails, type B, B answers anonymously, and onSave
sees a non-blank app password and creates me@B carrying A's credential,
which is exactly what onStartOver's own doc says must not happen.
Submitting an address now clears the credential, the discovery and the
host list with it, since all three belong to the address that produced
them.

A null serverRoot silently sent no credentials. serverRootFor cannot
parse a host-with-path like cloud.example.com/nextcloud, and the typed
password was then never put on the wire — while the resulting 401 was
reported as "credentials rejected" about a password nothing had tried.

hostsNeedingAuth was refreshed on the browser path but not on the typed
one, so it held hosts from the *unauthenticated* probe. An authenticated
PROPFIND reaches further — principal, then home sets — so the
cross-domain home set that actually caused the 401 is the one most
likely to be missing, and the user got "wrong password" instead of the
diagnostic naming it.

And the poll has the same uncancellable shape the revocation had:
execute() parks on a socket read, so cancelling the job only stops the
next request. It carries its own budget now, sized for a two-second
loop rather than for a multiget.
This commit is contained in:
makiolaj
2026-09-09 11:28:08 +02:00
parent 9c84b49fc9
commit 00026e698b
3 changed files with 156 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CalDavGateway.kt
+17 -1
View File
@@ -9,6 +9,7 @@ import de.jeanlucmakiola.caldav.NextcloudLoginFlow
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.withContext
import okhttp3.HttpUrl
import java.util.concurrent.TimeUnit
import javax.inject.Inject
import javax.inject.Singleton
@@ -84,9 +85,19 @@ class OkHttpCalDavGateway @Inject constructor(
.getOrNull()
}
/**
* ⚠️ The budget lives on the request, as it does for the revocation.
* `execute()` parks on a socket read that no cancellation can break, so the
* four places that cancel the poll job only stop the *next* request — and
* the shared client's ceiling is sized for a multiget, not for a two-second
* poll loop against a server that answered a moment ago.
*/
override suspend fun pollLoginFlow(flow: NextcloudLoginFlow.Flow): NextcloudLoginFlow.PollResult =
withContext(io) {
NextcloudLoginFlow(CalDavHttp.anonymous(userAgent), userAgent).poll(flow, now())
val client = CalDavHttp.anonymous(userAgent).newBuilder()
.callTimeout(POLL_TIMEOUT_SECONDS, TimeUnit.SECONDS)
.build()
NextcloudLoginFlow(client, userAgent).poll(flow, now())
}
override suspend fun revokeAppPassword(
@@ -108,4 +119,9 @@ class OkHttpCalDavGateway @Inject constructor(
}
private fun now() = System.currentTimeMillis() / 1000
private companion object {
/** One poll of a 2s loop. Long enough for a homelab, short enough to cancel. */
const val POLL_TIMEOUT_SECONDS = 15L
}
}
app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModel.kt
+44 -14
View File
@@ -138,6 +138,18 @@ class AddAccountViewModel @Inject constructor(
// *Continue*, not start over — so a second attempt would mint a second
// password on top of the first without this.
discardMintedPassword()
// ⚠️ And the credential itself, not just the minted one. backToServer is
// reachable *after* a successful approval — a post-approval discovery
// failure lands on an ordinary address step with a live Continue button
// — and only onStartOver cleared these. So: approve on server A,
// discovery fails, type server B, B answers anonymously, and onSave sees
// a non-blank password and creates me@B carrying A's credential.
username = ""
appPassword = ""
// Discovery belongs to the address that produced it, and so does the
// host list the diagnostics read.
found = null
hostsNeedingAuth = emptyList()
// The mismatch is sticky on purpose, but it belongs to the address that
// produced it — carrying it into a different server's flow makes a claim
// about that server's settings which was never measured.
@@ -192,33 +204,51 @@ class AddAccountViewModel @Inject constructor(
working(AddAccountMessage.Progress.SigningIn)
viewModelScope.launch {
val credentials = serverRoot?.let {
CalDavGateway.Credentials(username, appPassword, it)
}
// ⚠️ An error, not a silent unauthenticated retry. serverRootFor
// cannot parse a host-with-path like cloud.example.com/nextcloud, and
// passing null here sent no credentials at all — then reported the
// resulting 401 as "credentials rejected" about a password nothing
// had tried.
val root = serverRoot
?: return@launch backToServer(CalDavDiscovery.Outcome.Cause.NOT_AN_ADDRESS)
val credentials = CalDavGateway.Credentials(username, appPassword, root)
when (val outcome = gateway.discover(typedInput, credentials)) {
is CalDavDiscovery.Outcome.Found -> onDiscovered(outcome)
// A second rejection is the point at which naming the provider's
// own rule is worth more than repeating "wrong password".
is CalDavDiscovery.Outcome.NeedsAuthentication,
CalDavDiscovery.Outcome.Unauthenticated,
-> _state.update {
it.copy(
step = AddAccountStep.EnterCredentials(
username = username,
password = "",
error = quirkHint() ?: crossDomainHint()
?: AddAccountMessage.CredentialsRejected,
),
)
//
// ⚠️ Refreshed here too, not only on the browser path. An
// authenticated PROPFIND reaches further than the anonymous
// probe — principal, then home sets — so the cross-domain home
// set that actually caused this 401 is the one most likely to be
// missing from a list collected before the password was sent,
// and the user gets "credentials rejected" instead of the
// diagnostic that names it.
is CalDavDiscovery.Outcome.NeedsAuthentication -> {
hostsNeedingAuth = outcome.hosts
credentialsRejected()
}
CalDavDiscovery.Outcome.Unauthenticated -> credentialsRejected()
is CalDavDiscovery.Outcome.NotCalDav -> backToServer(outcome.cause)
is CalDavDiscovery.Outcome.Failed -> backToServer(outcome.cause)
}
}
}
private fun credentialsRejected() = _state.update {
it.copy(
step = AddAccountStep.EnterCredentials(
username = username,
password = "",
error = quirkHint() ?: crossDomainHint()
?: AddAccountMessage.CredentialsRejected,
),
)
}
/** Step 2b → the browser flow, if this looks like a Nextcloud. */
private suspend fun offerSignIn() {
val root = serverRoot ?: return backToServer(CalDavDiscovery.Outcome.Cause.NOT_AN_ADDRESS)
app/src/test/java/de/jeanlucmakiola/agendula/ui/accounts/AddAccountViewModelTest.kt
+95
View File
@@ -418,6 +418,60 @@ class AddAccountViewModelTest {
}
}
@Nested
inner class TheCredentialsStep {
@Test
fun `an address with no parseable root is an error, not a silent retry`() =
runTest(dispatcher) {
// serverRootFor cannot parse a host-with-path, and passing null
// sent no credentials at all — then reported the 401 as
// "credentials rejected" about a password nothing had tried.
gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList())
gateway.loginFlow = null
val vm = viewModel()
vm.onServerInputChanged("cloud.example.com/nextcloud")
vm.onServerSubmitted()
advanceUntilIdle()
vm.onUsernameChanged("me")
vm.onPasswordChanged("pw")
vm.onCredentialsSubmitted()
advanceUntilIdle()
val step = vm.state.value.step as AddAccountStep.EnterServer
assertThat(step.error).isEqualTo(CalDavDiscovery.Outcome.Cause.NOT_AN_ADDRESS)
// One discovery, the anonymous one. Nothing was sent blind.
assertThat(gateway.discoveries).hasSize(1)
}
@Test
fun `the authenticated pass refreshes the hosts the diagnostic reads`() =
runTest(dispatcher) {
// The anonymous probe stops at the root; an authenticated one
// reaches the principal and its home sets, so the cross-domain
// home set behind the 401 first appears here.
gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList())
gateway.loginFlow = null
gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(
listOf("dav.elsewhere.test"),
)
val vm = viewModel()
vm.onServerInputChanged("https://cloud.example.com/")
vm.onServerSubmitted()
advanceUntilIdle()
vm.onUsernameChanged("me")
vm.onPasswordChanged("pw")
vm.onCredentialsSubmitted()
advanceUntilIdle()
val step = vm.state.value.step as AddAccountStep.EnterCredentials
assertThat(step.error)
.isEqualTo(AddAccountMessage.OutsideCredentialScope("dav.elsewhere.test"))
}
}
@Nested
inner class ChoosingLists {
@@ -484,6 +538,47 @@ class AddAccountViewModelTest {
.isInstanceOf(AddAccountStep.EnterCredentials::class.java)
}
@Test
fun `a new address does not inherit the previous server's credentials`() =
runTest(dispatcher) {
// Approve on A, discovery fails afterwards, type B — which
// answers anonymously. The screen offers *Continue* here, not
// start over, so nothing else clears what A minted.
gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(
listOf("cloud.example.com"),
)
gateway.loginFlow = flow()
gateway.pollResults += NextcloudLoginFlow.PollResult.Approved(
NextcloudLoginFlow.Credentials(
server = "https://cloud.example.com/".toHttpUrl(),
loginName = "me",
appPassword = "a-pw",
),
)
gateway.discoveryOutcomes += CalDavDiscovery.Outcome.Failed(
CalDavDiscovery.Outcome.Cause.UNREACHABLE,
"no route to host",
)
val vm = viewModel()
vm.onServerInputChanged("https://cloud.example.com/")
vm.onServerSubmitted()
advanceUntilIdle()
gateway.loginFlow = null
gateway.discoveryOutcomes += found(collection("Tasks"))
vm.onServerInputChanged("https://other.example.com/")
vm.onServerSubmitted()
advanceUntilIdle()
vm.onSave()
advanceUntilIdle()
// ⚠️ Otherwise an account named me@other.example.com is created
// carrying cloud.example.com's app password.
assertThat(creator.created).isEmpty()
assertThat(vm.state.value.step)
.isInstanceOf(AddAccountStep.EnterCredentials::class.java)
}
@Test
fun `a failure inside create leaves a translatable message, not a spinner`() =
runTest(dispatcher) {