mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-06 03:38:23 +00:00
fix: keep the session-grant invariants with the state they protect
Three follow-ups from reviewing the session-grant feature. Each is a case where the rule was right but lived somewhere it could not hold. 1. "Never log in" clears the grants, but that pairing was written in the settings screen's onClick, which made it a property of one screen rather than of the account. Any other caller of AccountSettings.changeDefaultRelayAuthPolicy would silently reintroduce grants that outlive the switch-it-all-off answer — and a grant outranks the policy, so they would authenticate. Moved to Account.changeDefaultRelayAuthPolicy, which owns both the setting and the grants; the screen now calls that. Stored Always/Never exceptions are still left alone, since those outrank the policy by design and are listed. 2. RelayAuthPermissionLedger.sessionGrants defaulted to a fresh instance. Account passes the shared one, so nothing was broken, but the default meant a ledger built without it got a private set instead of failing — and this is shared state by construction: the foreground screen and the background notification consumer decide off one ledger, so a split set would lose answers between them and bring the dialog back. Now required. 3. Blocking a relay did not drop its session grant. Blocking outranks everything while it is in force, so nothing leaked, but lifting the block resumed authenticating off an answer given before it — and the weaker "never allow" already drops the grant, so the stronger signal not doing so was backwards. Account now observes the block list and revokes through the new RelayAuthPermissionLedger.revokeSessionGrantsFor. Observed rather than hooked onto the local block action because kind 10006 is shared: a block published by another client arrives as a flow update with no call of ours behind it. Verified on an emulator for 1 (selecting "never log in" still clears the grants through the new path) and by unit test for 2 and 3. The block list has no editor screen in this build — it is rendered from a published kind-10006 note — so 3's wiring is covered by its test plus the fact that both sides key off NormalizedRelayUrl.url, not by an end-to-end run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
99dc8c57ff
commit
30b48de304
@@ -64,6 +64,7 @@ import com.vitorpamplona.amethyst.commons.model.privateChats.hasEncryptedContent
|
||||
import com.vitorpamplona.amethyst.commons.relayClient.user.UserFinderAccount
|
||||
import com.vitorpamplona.amethyst.commons.relayauth.RelayAuthCustomToggles
|
||||
import com.vitorpamplona.amethyst.commons.relayauth.RelayAuthPermissionStore
|
||||
import com.vitorpamplona.amethyst.commons.relayauth.RelayAuthPolicy
|
||||
import com.vitorpamplona.amethyst.commons.richtext.RichTextParser
|
||||
import com.vitorpamplona.amethyst.commons.service.pow.PersistedPoWJob
|
||||
import com.vitorpamplona.amethyst.commons.service.pow.PoWCategory
|
||||
@@ -441,6 +442,27 @@ class Account(
|
||||
isVenueHostRelay = { relayUrl -> relayUrl.normalizeRelayUrlOrNull()?.let { it in venueHostRelays() } ?: false },
|
||||
)
|
||||
|
||||
/**
|
||||
* Sets the global NIP-42 policy, dropping every session grant when it becomes
|
||||
* [RelayAuthPolicy.NEVER].
|
||||
*
|
||||
* The two halves belong together, which is why they live here instead of in the settings screen
|
||||
* that used to pair them: a session grant outranks the policy (see
|
||||
* [com.vitorpamplona.amethyst.commons.relayauth.RelayAuthResolver]), so "never log in" only
|
||||
* means what it says if the casual one-tap answers go with it. As a composable's `onClick` that
|
||||
* was a property of one screen rather than of the account, and any other caller of
|
||||
* [AccountSettings.changeDefaultRelayAuthPolicy] silently reintroduced grants that outlive the
|
||||
* switch-it-all-off answer.
|
||||
*
|
||||
* Stored Always/Never exceptions are deliberately left alone: those outrank the policy by
|
||||
* design, and the settings screen lists them, so they are a standing answer rather than a
|
||||
* casual one.
|
||||
*/
|
||||
fun changeDefaultRelayAuthPolicy(policy: RelayAuthPolicy) {
|
||||
settings.changeDefaultRelayAuthPolicy(policy)
|
||||
if (policy == RelayAuthPolicy.NEVER) relayAuthSessionGrants.clear()
|
||||
}
|
||||
|
||||
/**
|
||||
* Relays that exist here because *this account* joined a room on them: the host of every NIP-29
|
||||
* relay group on its kind-10009 list, plus the relays of every Concord community on its
|
||||
@@ -3550,6 +3572,20 @@ class Account(
|
||||
init {
|
||||
Log.d("AccountRegisterObservers", "Init")
|
||||
|
||||
// Blocking a relay has to forget any "just for now" login to it, or unblocking later would
|
||||
// silently resume authenticating off an answer given before the block. Blocking is the
|
||||
// strongest signal available here — the weaker "never allow" already drops the grant via
|
||||
// RelayAuthPermissionLedger.setDecision, so it would be odd for the stronger one not to.
|
||||
//
|
||||
// Observed rather than hooked onto the local block action because the kind-10006 list is
|
||||
// shared: a block published by another client arrives as a flow update with no call of ours
|
||||
// behind it.
|
||||
scope.launch {
|
||||
blockedRelayList.flow.collect { blocked ->
|
||||
relayAuthLedger.revokeSessionGrantsFor(blocked.map { it.url })
|
||||
}
|
||||
}
|
||||
|
||||
// Start the Cashu wallet state observers AFTER all field initializers
|
||||
// complete — auto-redeem can fire as soon as start() returns, and it
|
||||
// calls back into sendLiterallyEverywhere which depends on
|
||||
|
||||
+19
-3
@@ -53,10 +53,15 @@ class RelayAuthPermissionLedger(
|
||||
/**
|
||||
* Relays this account already approved during this run of the app. Answering the prompt without
|
||||
* the "remember" switch records the grant here, so the same relay's next reconnect is answered
|
||||
* silently instead of raising the same dialog again. Empty by default — a ledger built without
|
||||
* one simply has no session memory.
|
||||
* silently instead of raising the same dialog again.
|
||||
*
|
||||
* Required, with no default, because it is shared state: one account's grants have to be the
|
||||
* same object on every AUTH path (the foreground screen and the background notification
|
||||
* consumer both decide off this ledger — see [com.vitorpamplona.amethyst.model.Account]). A
|
||||
* default would let a ledger built without one quietly get a private set instead, so answers
|
||||
* given on one path would not be seen on the other and the dialog would come back anyway.
|
||||
*/
|
||||
val sessionGrants: RelayAuthSessionGrants = RelayAuthSessionGrants(),
|
||||
val sessionGrants: RelayAuthSessionGrants,
|
||||
val customToggles: () -> RelayAuthCustomToggles = { RelayAuthCustomToggles() },
|
||||
val isInMyRelayList: (String) -> Boolean = { false },
|
||||
val isBlocked: (String) -> Boolean = { false },
|
||||
@@ -171,6 +176,17 @@ class RelayAuthPermissionLedger(
|
||||
/** Forgets this session's grant for [relayUrl], so the next challenge is decided from scratch. */
|
||||
fun revokeSessionGrant(relayUrl: String) = sessionGrants.revoke(relayUrl)
|
||||
|
||||
/**
|
||||
* Forgets this session's grants for every relay in [blockedRelayUrls] — the kind-10006 block
|
||||
* list, which outranks everything else on the decision path.
|
||||
*
|
||||
* Blocking already denies while it is in force, so this is about what happens *after* it is
|
||||
* lifted: without it, unblocking would resume authenticating off an answer given before the
|
||||
* block. The weaker "never allow" drops the grant too (see [setDecision]), so the stronger
|
||||
* signal has to as well.
|
||||
*/
|
||||
fun revokeSessionGrantsFor(blockedRelayUrls: Collection<String>) = blockedRelayUrls.forEach(sessionGrants::revoke)
|
||||
|
||||
/**
|
||||
* Stores a per-relay override for [relayUrl].
|
||||
*
|
||||
|
||||
+4
-8
@@ -241,14 +241,10 @@ fun RelayAuthSettingsScreen(
|
||||
selected = globalPolicy == policy,
|
||||
title = stringResource(titleRes),
|
||||
description = stringResource(descRes),
|
||||
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()
|
||||
},
|
||||
// Account, not settings: choosing "never log in" also drops this
|
||||
// session's grants, and that pairing is the account's rule rather
|
||||
// than this screen's. See Account.changeDefaultRelayAuthPolicy.
|
||||
onClick = { account.changeDefaultRelayAuthPolicy(policy) },
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
+1
-1
@@ -69,7 +69,7 @@ class RelayAuthGrantRationaleTest {
|
||||
private val bob = "b".repeat(64)
|
||||
private val carol = "c".repeat(64)
|
||||
|
||||
private fun ledger(store: RelayAuthPermissionStore) = RelayAuthPermissionLedger(store, { RelayAuthPolicy.CUSTOM })
|
||||
private fun ledger(store: RelayAuthPermissionStore) = RelayAuthPermissionLedger(store, { RelayAuthPolicy.CUSTOM }, RelayAuthSessionGrants())
|
||||
|
||||
@Test
|
||||
fun recordsCounterpartiesGroupedByPurpose() =
|
||||
|
||||
+18
@@ -289,4 +289,22 @@ class RelayAuthSessionGrantsTest {
|
||||
assertTrue(ledger.grantForSession(relay))
|
||||
assertEquals(RelayAuthVerdict.DENY, ledger.decide(askable(relay)))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun blockingARelayForgetsItsSessionGrantSoUnblockingDoesNotResumeIt() =
|
||||
runTest {
|
||||
// While the block is in force the relay is denied whatever the grant says, so what this
|
||||
// pins is the state left behind for when the block is lifted.
|
||||
val grants = RelayAuthSessionGrants()
|
||||
val ledger = ledger(grants = grants)
|
||||
|
||||
ledger.grantForSession(relay)
|
||||
ledger.grantForSession(other)
|
||||
|
||||
ledger.revokeSessionGrantsFor(listOf(relay))
|
||||
|
||||
assertEquals(setOf(other), grants.grants.value)
|
||||
assertEquals(RelayAuthVerdict.ASK, ledger.decide(askable(relay)))
|
||||
assertEquals(RelayAuthVerdict.ALLOW, ledger.decide(askable(other)))
|
||||
}
|
||||
}
|
||||
|
||||
+1
@@ -62,6 +62,7 @@ class RelayAuthVenueCoverageTest {
|
||||
RelayAuthPermissionLedger(
|
||||
store = NoStore(),
|
||||
globalPolicy = { RelayAuthPolicy.CUSTOM },
|
||||
sessionGrants = RelayAuthSessionGrants(),
|
||||
customToggles = { toggles },
|
||||
isTrustedVenue = { _, venueId -> venueId == joinedGroupId || venueId == joinedCommunityId },
|
||||
isVenueHostRelay = { it == groupRelay || it == concordRelay },
|
||||
|
||||
Reference in New Issue
Block a user