diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPermissionLedger.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPermissionLedger.kt index 2aefc81fca..aa45baf7fa 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPermissionLedger.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthPermissionLedger.kt @@ -161,13 +161,23 @@ class RelayAuthPermissionLedger( * Also drops any session grant: the stored decision is now the whole answer for this relay, so * leaving the transient one behind would let a later [clearDecision] ("follows your rules again") * silently keep authenticating off a grant the user can no longer see. + * + * The two writes are not atomic — [RelayAuthPermissionCache] only publishes an override to memory + * *after* its disk write returns — so a challenge arriving between them must never see neither. + * Which side to fail on depends on the decision: + * - **DENY** revokes first. The window then asks or denies, never signs: a user who just pressed + * "never allow" must not get one more AUTH out of the grant they are replacing. + * - **ALLOW** revokes last, so the grant still covers the window. Revoking first left the relay + * momentarily undecided, which re-prompted the user who had just pressed "always" — the very + * dialog this whole feature exists to stop. */ suspend fun setDecision( relayUrl: String, decision: RelayAuthDecision, ) { - sessionGrants.revoke(relayUrl) + if (decision == RelayAuthDecision.DENY) sessionGrants.revoke(relayUrl) store.storeDecision(relayUrl, decision) + sessionGrants.revoke(relayUrl) } /** Removes the per-relay override for [relayUrl], reverting to the global policy. */ diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/relayauth/RelayAuthSettingsScreen.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/relayauth/RelayAuthSettingsScreen.kt index aa02af4420..8d084160d8 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/relayauth/RelayAuthSettingsScreen.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/relayauth/RelayAuthSettingsScreen.kt @@ -230,7 +230,14 @@ fun RelayAuthSettingsScreen( selected = globalPolicy == policy, title = stringResource(titleRes), description = stringResource(descRes), - onClick = { account.settings.changeDefaultRelayAuthPolicy(policy) }, + onClick = { + account.settings.changeDefaultRelayAuthPolicy(policy) + // "Never log in" is the switch-it-all-off answer, so a casual + // "just for now" tap must not quietly outlive it. Deliberate + // Always/Never exceptions are left alone — those outrank the + // global policy by design, and the list above says so. + if (policy == RelayAuthPolicy.NEVER) account.relayAuthSessionGrants.clear() + }, ) } } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthSessionGrantsTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthSessionGrantsTest.kt index 0d941b4b49..b9285192c8 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthSessionGrantsTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthSessionGrantsTest.kt @@ -24,11 +24,16 @@ import com.vitorpamplona.amethyst.commons.relayauth.AuthPurpose import com.vitorpamplona.amethyst.commons.relayauth.AuthPurposeKind import com.vitorpamplona.amethyst.commons.relayauth.RelayAuthContext import com.vitorpamplona.amethyst.commons.relayauth.RelayAuthDecision +import com.vitorpamplona.amethyst.commons.relayauth.RelayAuthPermissionStore import com.vitorpamplona.amethyst.commons.relayauth.RelayAuthPolicy import com.vitorpamplona.amethyst.commons.relayauth.RelayAuthVerdict +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse +import org.junit.Assert.assertNotEquals import org.junit.Assert.assertTrue import org.junit.Test @@ -46,7 +51,7 @@ class RelayAuthSessionGrantsTest { private fun ledger( grants: RelayAuthSessionGrants = RelayAuthSessionGrants(), - store: InMemoryRelayAuthPermissionStore = InMemoryRelayAuthPermissionStore(), + store: RelayAuthPermissionStore = InMemoryRelayAuthPermissionStore(), blocked: Set = emptySet(), ) = RelayAuthPermissionLedger( store = store, @@ -157,6 +162,63 @@ class RelayAuthSessionGrantsTest { assertEquals(RelayAuthVerdict.ASK, ledger.decide(askable(relay))) } + /** + * A store whose write suspends until [gate] opens, modelling the real one: the disk write is + * awaited *before* [RelayAuthPermissionCache] publishes the new override to memory, so there is a + * window where the override is not yet readable. + */ + private class GatedStore( + private val gate: CompletableDeferred, + private val inner: RelayAuthPermissionStore = InMemoryRelayAuthPermissionStore(), + ) : RelayAuthPermissionStore by inner { + override suspend fun storeDecision( + relayUrl: String, + decision: RelayAuthDecision, + ) { + gate.await() + inner.storeDecision(relayUrl, decision) + } + } + + @Test + fun promotingAGrantToAlwaysNeverOpensAGapThatRePrompts() = + runTest { + val gate = CompletableDeferred() + val ledger = ledger(store = GatedStore(gate)) + ledger.grantForSession(relay) + + val write = launch { ledger.setDecision(relay, RelayAuthDecision.ALLOW) } + runCurrent() + + // Mid-write the override is not readable yet. If the grant has already been dropped the + // relay is momentarily undecided and a reconnect lands a fresh dialog on a user who just + // pressed "Always" — the exact prompt this feature exists to stop. + assertEquals(RelayAuthVerdict.ALLOW, ledger.decide(askable(relay))) + + gate.complete(Unit) + write.join() + assertEquals(RelayAuthVerdict.ALLOW, ledger.decide(askable(relay))) + } + + @Test + fun neverAllowStopsAuthenticatingBeforeItsWriteLands() = + runTest { + val gate = CompletableDeferred() + val ledger = ledger(store = GatedStore(gate)) + ledger.grantForSession(relay) + + val write = launch { ledger.setDecision(relay, RelayAuthDecision.DENY) } + runCurrent() + + // The opposite bias to the ALLOW case: a user who just said "never" must not have one more + // AUTH signed on the strength of the grant they are replacing. + assertNotEquals(RelayAuthVerdict.ALLOW, ledger.decide(askable(relay))) + + gate.complete(Unit) + write.join() + assertEquals(RelayAuthVerdict.DENY, ledger.decide(askable(relay))) + } + @Test fun grantsAreObservableForTheSettingsScreen() { val grants = RelayAuthSessionGrants()