mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-06 03:38:23 +00:00
fix: close two gaps in the relay auth session grant
Audit follow-up to the previous commit. 1. setDecision revoked the in-memory grant before awaiting the store write, but RelayAuthPermissionCache only publishes an override to memory after its disk write returns. A challenge landing between the two saw neither the grant nor the override, fell through to the policy, and re-prompted — a fresh dialog for a user who had just pressed "Always", which is the exact prompt the feature exists to remove. Fixed with asymmetric ordering, because the two decisions want opposite bias: ALLOW revokes last so the grant covers the window, while DENY revokes first so the window asks or denies but never signs — someone who just pressed "never allow" must not get one more AUTH out of the grant they are replacing. clearDecision needs no change: the old override stays readable across its window, so no gap exists. Covered by a gated store that suspends mid-write; the ALLOW case fails without the reorder. 2. Switching the global policy to "Never log in" left previously granted relays authenticating, since the grant is checked before the policy. Stored exceptions outranking the policy is deliberate and documented, but a casual one-tap grant surviving the switch-it-all-off answer is not the same claim. Clearing grants on NEVER also puts RelayAuthSessionGrants.clear() to use, which was otherwise unreferenced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rado2dnqpbCuCUyCd3trQz
This commit is contained in:
+11
-1
@@ -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. */
|
||||
|
||||
+8
-1
@@ -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()
|
||||
},
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
+63
-1
@@ -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<String> = 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<Unit>,
|
||||
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<Unit>()
|
||||
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<Unit>()
|
||||
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()
|
||||
|
||||
Reference in New Issue
Block a user