diff --git a/amethyst/plans/2026-08-03-auth-permissions-redesign.md b/amethyst/plans/2026-08-03-auth-permissions-redesign.md index 02c2c519f0..ae69a30b65 100644 --- a/amethyst/plans/2026-08-03-auth-permissions-redesign.md +++ b/amethyst/plans/2026-08-03-auth-permissions-redesign.md @@ -38,9 +38,23 @@ Shipped as designed. Where it diverged or went further: where the new sentence belongs — or silently dropped a new `%1$s`. New keys fall back to the new English until Crowdin catches up. The 401 now-orphaned translations were deleted from the 11 locale files that carried them. -- **Not done:** the 60-second silent `DISMISS` (item 6 below) is still a silent - deny with the event left pending in the outbox. It needs a decision about what - the user should see, not just a layout. +- **The 60s timeout now runs from display, not from arrival.** Found while + answering "what happens if several auths are requested inside one 60s window?". + The host shows one dialog at a time but every prompt's deadline started when its + challenge arrived, so a burst of relays meant prompt 2..N counted down while + invisible. They expired unseen — a silent deny — and if the user did reach one + after it expired, the click was swallowed whole: `complete()` is a no-op on a + resolved deferred, so no auth was sent and not even the "always allow" rule was + written. `RelayAuthPrompt.markShown()` now starts the window, gated on a host + actually collecting (no UI → the old arrival clock, which is what the timeout was + always for) and capped by `queueWaitMs` so a stuck queue can't suspend a + connection forever. A second challenge for the same (relay, account) rides along + on the owner's answer with no deadline of its own — running one would let it + resolve the shared deferred and tear down a dialog mid-read. +- **Still not done:** what a timeout should *look like*. It is now an honest 60s of + visible time rather than a clock the user never saw, but it is still a dialog + that vanishes and an event left pending in the outbox with no feedback. That + needs a product decision, not a layout. Verified: `:amethyst:testFdroidDebugUnitTest` 1096 tests green (38 in the relay-auth suites, 5 of them new), `:commons:jvmTest` 1446 green, `spotlessApply` clean. diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/compose/RelayAuthPromptHost.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/compose/RelayAuthPromptHost.kt index 7315dde3ad..244f9b043a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/compose/RelayAuthPromptHost.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/compose/RelayAuthPromptHost.kt @@ -113,7 +113,10 @@ fun RelayAuthPromptHost( } } + // One dialog at a time. Everything else waits its turn, and tells the bus when its turn comes so + // its answer window starts from the moment it is visible rather than from the challenge. queue.firstOrNull { !it.isResolved }?.let { prompt -> + LaunchedEffect(prompt) { prompt.markShown() } RelayAuthPromptDialog(prompt, accountViewModel, nav) { choice -> prompt.respond(choice) queue.remove(prompt) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBus.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBus.kt index 61cdeeb943..a8ad8ea46a 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBus.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBus.kt @@ -26,6 +26,7 @@ import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.SharedFlow +import kotlinx.coroutines.flow.first import kotlinx.coroutines.withTimeoutOrNull /** What the user chose when asked whether to authenticate with a relay. */ @@ -64,6 +65,20 @@ class RelayAuthPrompt( reply.complete(choice) } + private val shown = CompletableDeferred() + + /** + * Called by the host the moment this prompt is actually put on screen. The answer window is + * measured from here, not from when the challenge arrived — the host shows one dialog at a time, + * so a prompt can sit queued behind another for a long while, and its clock must not be running + * during that. + */ + fun markShown() { + shown.complete(Unit) + } + + internal suspend fun awaitShown() = shown.await() + /** True once answered by the user or resolved by the bus (e.g. timed out). */ val isResolved: Boolean get() = reply.isCompleted @@ -82,6 +97,7 @@ class RelayAuthPrompt( */ class RelayAuthPromptBus( private val timeoutMs: Long = DEFAULT_TIMEOUT_MS, + private val queueWaitMs: Long = DEFAULT_QUEUE_WAIT_MS, ) { // replay so a challenge raised *before* the UI host subscribes — cold start, an account switch, // any moment no RelayAuthPromptHost is collecting — isn't dropped (which would stall the auth @@ -112,16 +128,58 @@ class RelayAuthPromptBus( ?: CompletableDeferred().also { inFlight[key] = it } to true } - if (isOwner) mutablePrompts.emit(RelayAuthPrompt(relayUrl, purposes, askingAccount, isMyOwnRelay, deferred)) + // Only the owner surfaces a dialog and owns its deadline. A second challenge for the same + // (relay, account) just rides along on the owner's answer — it must NOT run a deadline of its + // own, because resolving the shared deferred would tear down a dialog the user is still + // looking at. + if (!isOwner) return rideAlong(deferred) + + val prompt = RelayAuthPrompt(relayUrl, purposes, askingAccount, isMyOwnRelay, deferred) + mutablePrompts.emit(prompt) return try { - awaitOrTimeout(deferred) + awaitOrTimeout(prompt, deferred) } finally { - if (isOwner) synchronized(inFlight) { inFlight.remove(key) } + synchronized(inFlight) { inFlight.remove(key) } } } - private suspend fun awaitOrTimeout(deferred: CompletableDeferred): UserAuthChoice { + /** + * Waits for the owner's answer without imposing a deadline that could resolve the shared prompt. + * The cap exists only so a cancelled owner can't strand this caller forever; it deliberately + * returns [UserAuthChoice.DISMISS] *locally* rather than completing the deferred. + */ + private suspend fun rideAlong(deferred: CompletableDeferred): UserAuthChoice = withTimeoutOrNull(queueWaitMs + 2 * timeoutMs) { deferred.await() } ?: UserAuthChoice.DISMISS + + /** + * Waits for the user's answer, giving them [timeoutMs] **from the moment the dialog is on screen** + * rather than from the moment the challenge arrived. + * + * That distinction is the whole point. The host renders one dialog at a time, so when several + * relays challenge at once every prompt but the first is queued and invisible — and with a single + * deadline measured from arrival, those queued prompts expired without ever being shown. The user + * saw nothing, the relay was silently denied, and a click on a dialog that had already expired was + * swallowed whole: `complete()` is a no-op on a resolved deferred, so the auth was never sent and + * not even the "always allow" rule was written. + */ + private suspend fun awaitOrTimeout( + prompt: RelayAuthPrompt, + deferred: CompletableDeferred, + ): UserAuthChoice { + // Is anything able to display this? With no host collecting — a headless background process, + // or a cold start before the UI subscribes — nobody can ever answer, and the relay coroutine + // must not hang. That case is what the timeout has always been for, so it keeps the old clock. + val hasHost = withTimeoutOrNull(timeoutMs) { mutablePrompts.subscriptionCount.first { it > 0 } } != null + + if (hasHost) { + // Wait for this prompt's turn at the front of the host's queue. Capped, so a host that + // stops rendering (backgrounded mid-queue) still can't suspend the connection forever. + withTimeoutOrNull(queueWaitMs) { prompt.awaitShown() } + } + + // The answer window proper. If the user already answered while we were waiting above, this + // returns immediately. withTimeoutOrNull(timeoutMs) { deferred.await() }?.let { return it } + // Timed out: resolve the deferred so any UI still showing this prompt can drop it, and so a // concurrent waiter on the same deferred gets an answer too. complete() is a no-op if a late // user response already won the race. @@ -131,5 +189,8 @@ class RelayAuthPromptBus( companion object { const val DEFAULT_TIMEOUT_MS = 60_000L + + /** How long a prompt may wait its turn behind other dialogs before we give up on it. */ + const val DEFAULT_QUEUE_WAIT_MS = 5 * 60_000L } } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBusTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBusTest.kt index 176cdeb684..d1931883bf 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBusTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPromptBusTest.kt @@ -23,6 +23,7 @@ package com.vitorpamplona.amethyst.service.relayClient.authCommand.model import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.async +import kotlinx.coroutines.delay import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.take import kotlinx.coroutines.flow.toList @@ -113,4 +114,65 @@ class RelayAuthPromptBusTest { bus.prompts.first().respond(UserAuthChoice.ALLOW_ONCE) assertEquals(UserAuthChoice.ALLOW_ONCE, caller.await()) } + + @Test + fun aQueuedPromptsClockStartsWhenItIsShown() = + runTest { + val bus = RelayAuthPromptBus(timeoutMs = 1_000L) + val relayA = NormalizedRelayUrl("wss://a.relay.test") + val relayB = NormalizedRelayUrl("wss://b.relay.test") + + val surfaced = async { bus.prompts.take(2).toList() } + val callerA = async { bus.requestDecision(relayA, emptyList(), alice, isMyOwnRelay = false) } + val callerB = async { bus.requestDecision(relayB, emptyList(), alice, isMyOwnRelay = false) } + val prompts = surfaced.await() + + // The host renders one dialog at a time, so B sits queued and invisible while the user + // works through A. Only A is on screen, so only A's clock is running. + prompts[0].markShown() + delay(600) + prompts[0].respond(UserAuthChoice.ALLOW_ONCE) + assertEquals(UserAuthChoice.ALLOW_ONCE, callerA.await()) + + // B's turn. Its own window opens now — well past the point where a single deadline + // measured from the challenge would already have expired it unseen. + prompts[1].markShown() + delay(600) + prompts[1].respond(UserAuthChoice.ALWAYS_ALLOW) + + assertEquals(UserAuthChoice.ALWAYS_ALLOW, callerB.await()) + } + + @Test + fun aPromptNoHostCanEverShowStillTimesOut() = + runTest { + // Nothing collects the flow, so nobody can answer. The relay coroutine must not hang + // waiting for a dialog that will never appear. + val bus = RelayAuthPromptBus(timeoutMs = 1_000L, queueWaitMs = 60_000L) + + assertEquals(UserAuthChoice.DISMISS, bus.ask()) + } + + @Test + fun aSecondChallengeNeverResolvesTheDialogTheUserIsLookingAt() = + runTest { + val bus = RelayAuthPromptBus(timeoutMs = 1_000L) + + val surfaced = async { bus.prompts.first() } + val owner = async { bus.ask() } + val rider = async { bus.ask() } + val prompt = surfaced.await() + + // The prompt is queued behind another dialog well past the rider's old deadline. The + // rider has no dialog of its own, so it must simply wait: if it ran its own clock it + // would complete the shared deferred at 1000ms and yank this prompt away before the + // user ever saw it. + delay(1_500) + prompt.markShown() + delay(300) + prompt.respond(UserAuthChoice.ALWAYS_ALLOW) + + assertEquals(UserAuthChoice.ALWAYS_ALLOW, owner.await()) + assertEquals(UserAuthChoice.ALWAYS_ALLOW, rider.await()) + } }