From 30b48de304fe90b31fbfe428c135fc83d562c0d3 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Wed, 19 Aug 2026 13:04:55 -0400 Subject: [PATCH] fix: keep the session-grant invariants with the state they protect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../vitorpamplona/amethyst/model/Account.kt | 36 +++++++++++++++++++ .../model/RelayAuthPermissionLedger.kt | 22 ++++++++++-- .../relayauth/RelayAuthSettingsScreen.kt | 12 +++---- .../model/RelayAuthGrantRationaleTest.kt | 2 +- .../model/RelayAuthSessionGrantsTest.kt | 18 ++++++++++ .../model/RelayAuthVenueCoverageTest.kt | 1 + 6 files changed, 79 insertions(+), 12 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt index b763402e07..600fb8bf02 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/model/Account.kt @@ -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 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 81caf47040..ec6df47d4e 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 @@ -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) = blockedRelayUrls.forEach(sessionGrants::revoke) + /** * Stores a per-relay override for [relayUrl]. * 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 55f210a31b..187d3798f9 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 @@ -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) }, ) } } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthGrantRationaleTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthGrantRationaleTest.kt index 8b24f098cb..f3068d9195 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthGrantRationaleTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthGrantRationaleTest.kt @@ -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() = 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 c3290c9876..0ae3d10c76 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 @@ -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))) + } } diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthVenueCoverageTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthVenueCoverageTest.kt index 6b423aa049..f34e6a59f9 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthVenueCoverageTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/service/relayClient/authCommand/model/RelayAuthVenueCoverageTest.kt @@ -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 },