laantungir/amethyst
1
0
Fork 0
mirror of https://github.com/vitorpamplona/amethyst.git synced 2026-08-11 08:47:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(quartz): remove pendingNewlyAdded — commit auth once, not commit-then-rollback

Browse Source 
The pendingNewlyAdded field was a rollback token faked through instance state:
FullAuthPolicy.accept(AuthCmd) committed the pubkey eagerly, so a later
rejection (composed policy or a throwing hook) had to be undone, and the field
existed only to avoid dropping a pubkey that was already authenticated.

Fix the root cause — the eager commit. accept(AuthCmd) now only validates; the
pubkey is recorded in FullAuthPolicy.onAuthenticated (made final), which the
engine calls only after accept AND the whole policy chain approve the AUTH.
External-auth bridges override a new open `authorize` hook that runs before the
commit; throwing rejects the login with nothing committed to undo.

This deletes pendingNewlyAdded, IRelayPolicy.onAuthenticationFailed, its
PolicyStack override, and both rollback call-sites in RelaySession.handleAuth —
and makes the prior compose-after-reject / failed-re-AUTH cases correct by
construction (no rollback to get wrong). Tests updated to override `authorize`;
the two regression tests pass unchanged.

https://claude.ai/code/session_016YBS2pWCBSDgAMthHzfCTr
This commit is contained in:
Claude
2026-06-03 20:25:12 +00:00
parent a21f811e87
commit 9a19a0f346
6 changed files with 62 additions and 83 deletions
Download Patch File Download Diff File Expand all files Collapse all files
quartz/RELAY.md
+2 -2
View File
@@ -169,14 +169,14 @@ val server = NostrServer(
)
```
`FullAuthPolicy` already implements the full NIP-42 challenge/verify handshake — you should not re-implement it. To bridge auth to an external system (e.g. exchange the verified event for a backend JWT), override the `suspend` `onAuthenticated` hook. It runs after the NIP-42 checks pass but before the success `OK` is sent, so it can do network/disk I/O; throwing from it turns the AUTH into a failing `OK false` **and rolls the authentication back** (the connection is not left logged in behind a false `OK`):
`FullAuthPolicy` already implements the full NIP-42 challenge/verify handshake — you should not re-implement it. To bridge auth to an external system (e.g. exchange the verified event for a backend JWT), override the `suspend` `authorize` hook. It runs after the NIP-42 checks pass (and after the rest of the policy chain approves), and the pubkey is recorded only once it returns — so it can do network/disk I/O, and throwing from it rejects the login (`OK false`) with the connection left unauthenticated (nothing was committed):
```kotlin
class JwtAuthPolicy(
relay: NormalizedRelayUrl,
private val backend: AuthBackend,
) : FullAuthPolicy(relay) {
override suspend fun onAuthenticated(pubKey: HexKey, event: RelayAuthEvent) {
override suspend fun authorize(pubKey: HexKey, event: RelayAuthEvent) {
// Suspends; a thrown exception rejects the login with OK false.
backend.exchangeForSession(pubKey, event)
}
quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/IRelayPolicy.kt
+11 -23
View File
@@ -70,20 +70,19 @@ interface IRelayPolicy {
fun accept(cmd: AuthCmd): PolicyResult<AuthCmd>
/**
* Called after an AUTH command has been [accept]ed, before the success
* `OK` is sent. This is the place to run any post-authentication side
* effects that need network or disk I/O — for example, exchanging the
* verified NIP-42 event for a backend session token — without leaking
* that logic out of the policy and into the transport layer.
* Called once an AUTH command has been [accept]ed by this policy *and* the
* rest of the policy chain, before the success `OK` is sent. This is where
* a policy commits the authentication and/or runs post-verification side
* effects that need network or disk I/O — e.g. exchanging the verified
* NIP-42 event for a backend session token — without leaking that logic into
* the transport layer.
*
* The synchronous [accept] does the cheap, deterministic NIP-42 checks
* (challenge, relay, timestamp); this suspend hook does the expensive,
* external part. Throwing here turns the AUTH into a failing `OK false`,
* so a bridge can reject a verified-but-unauthorized user by throwing.
* Because it runs only after the whole chain approved the AUTH, throwing
* here cleanly fails the login: the AUTH becomes `OK false` and, since the
* commit lives here too, the connection is never left authenticated. The
* default implementation does nothing.
*
* The default implementation does nothing.
*
* @param pubKey The pubkey that just authenticated.
* @param pubKey The pubkey being authenticated.
* @param event The verified NIP-42 auth event.
*/
suspend fun onAuthenticated(
@@ -91,17 +90,6 @@ interface IRelayPolicy {
event: RelayAuthEvent,
) {}
/**
* Rolls back an authentication that [accept] granted but [onAuthenticated]
* then rejected by throwing. The engine calls this so the connection does
* not stay authenticated after a failing `OK` — preserving the invariant
* that a client treated as logged in is exactly one that received `OK true`.
*
* The default implementation does nothing; [com.vitorpamplona.quartz.nip01Core.relay.server.policies.FullAuthPolicy]
* overrides it to drop the pubkey from its authenticated set.
*/
fun onAuthenticationFailed(pubKey: HexKey) {}
/**
* Inspects a raw inbound message before it is parsed. Return a reason
* string to reject it (the engine sends it as a `NOTICE`), or null to let
quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/RelaySession.kt
+4 -17
View File
@@ -201,32 +201,19 @@ class RelaySession(
private suspend fun handleAuth(cmd: AuthCmd) {
val result = policy.accept(cmd)
if (result is PolicyResult.Rejected) {
// A composed policy may have run *after* one that already committed
// the authentication (e.g. FullAuthPolicy) and then rejected. Roll
// back so a rejected AUTH never leaves the connection authenticated.
policy.onAuthenticationFailed(cmd.event.pubKey)
send(OkMessage(cmd.event.id, false, result.reason))
return
}
// Cheap NIP-42 checks passed. Run the policy's post-auth hook
// (which may do network/disk I/O, e.g. exchange the verified
// event for a backend session token) before confirming. A
// throw turns the AUTH into a failing OK so the client knows
// the login did not complete.
// The whole policy chain validated the AUTH. onAuthenticated runs any
// post-verification I/O (e.g. exchanging the verified event for a
// backend token) AND is where a policy commits the authentication, so a
// throw here cleanly fails the login — nothing was committed to undo.
try {
policy.onAuthenticated(cmd.event.pubKey, cmd.event)
} catch (e: CancellationException) {
throw e
} catch (e: Exception) {
// The hook rejected the login: undo whatever accept() committed
// so the connection isn't left authenticated behind a false OK.
// Guard the rollback so a misbehaving policy can't also swallow
// the failing OK and leave the client without a reply.
try {
policy.onAuthenticationFailed(cmd.event.pubKey)
} catch (_: Exception) {
}
send(OkMessage(cmd.event.id, false, "error: ${e.message ?: "authentication failed"}"))
return
}
quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/policies/FullAuthPolicy.kt
+40 -32
View File
@@ -32,6 +32,7 @@ import com.vitorpamplona.quartz.nip01Core.relay.normalizer.NormalizedRelayUrl
import com.vitorpamplona.quartz.nip01Core.relay.server.IRelayPolicy
import com.vitorpamplona.quartz.nip01Core.relay.server.PolicyResult
import com.vitorpamplona.quartz.nip40Expiration.isExpired
import com.vitorpamplona.quartz.nip42RelayAuth.RelayAuthEvent
import com.vitorpamplona.quartz.utils.RandomInstance
import com.vitorpamplona.quartz.utils.TimeUtils
@@ -39,14 +40,18 @@ import com.vitorpamplona.quartz.utils.TimeUtils
* Requires authentication for all EVENT, REQ, and COUNT commands.
* Replicates the previous `requireAuth = true` behavior.
*
* This class already implements the full NIP-42 challenge/verify handshake:
* [onConnect] sends the [challenge] and [accept] (AuthCmd) validates the
* returned event (expiration, freshness, challenge match, relay match) before
* recording the pubkey in [authenticatedUsers]. Subclasses generally should
* NOT re-implement that — to bridge to an external auth system, override
* [onAuthenticated] (a `suspend` hook) and do the post-verification I/O there,
* e.g. exchange the verified event for a backend session token. Throwing from
* that hook turns the AUTH into a failing `OK false`.
* Implements the full NIP-42 challenge/verify handshake: [onConnect] sends the
* [challenge] and [accept] (AuthCmd) validates the returned event (expiration,
* freshness, challenge match, relay match). Crucially, [accept] does NOT mutate
* state — the pubkey is recorded in [authenticatedUsers] only by [onAuthenticated],
* which the engine calls once [accept] *and* the rest of the policy chain have
* approved the AUTH. That single, late commit is why there is no rollback to
* reason about: a rejected AUTH simply never reaches it.
*
* To bridge to an external auth system, override [authorize] (a `suspend` hook)
* and do the post-verification I/O there — e.g. exchange the verified event for
* a backend session token. Throwing from it rejects the login (`OK false`) and
* the pubkey is never recorded.
*/
open class FullAuthPolicy(
val relay: NormalizedRelayUrl,
@@ -57,14 +62,6 @@ open class FullAuthPolicy(
/** Set of pubkeys that have successfully authenticated on this session. */
val authenticatedUsers = mutableSetOf<HexKey>()
/**
* The pubkey, if any, that the most recent [accept] added to
* [authenticatedUsers] for the first time. Lets [onAuthenticationFailed]
* roll back exactly what the failing AUTH committed without dropping a
* pubkey that was already authenticated on this connection.
*/
private var pendingNewlyAdded: HexKey? = null
/** Returns true if at least one pubkey has authenticated. */
fun isAuthenticated(): Boolean = authenticatedUsers.isNotEmpty()
@@ -74,7 +71,6 @@ open class FullAuthPolicy(
override fun accept(cmd: AuthCmd): PolicyResult<AuthCmd> {
val event = cmd.event
pendingNewlyAdded = null
if (event.isExpired()) {
return PolicyResult.Rejected("invalid: auth event expired")
@@ -92,15 +88,36 @@ open class FullAuthPolicy(
return PolicyResult.Rejected("invalid: relay url does not match")
}
// add() returns true only when the pubkey wasn't already present, so a
// later rollback removes only this AUTH's contribution.
if (authenticatedUsers.add(event.pubKey)) {
pendingNewlyAdded = event.pubKey
}
return PolicyResult.Accepted(cmd)
}
/**
* Commits the authentication. The engine calls this only after [accept] and
* the whole policy chain have approved the AUTH, so this is the single point
* where the pubkey is recorded. It runs [authorize] first (which may throw
* to reject) and records the pubkey only on success — the connection is
* never left authenticated behind a failing `OK`. `final`: override
* [authorize], not this.
*/
final override suspend fun onAuthenticated(
pubKey: HexKey,
event: RelayAuthEvent,
) {
authorize(pubKey, event)
authenticatedUsers.add(pubKey)
}
/**
* Hook for external authorization once the NIP-42 proof checks out — e.g.
* exchange [event] for a backend session token. Throw to reject the login
* (the AUTH becomes `OK false` and the pubkey is not recorded). Runs before
* the pubkey is committed. The default does nothing.
*/
open suspend fun authorize(
pubKey: HexKey,
event: RelayAuthEvent,
) {}
override fun accept(cmd: EventCmd): PolicyResult<EventCmd> =
if (isAuthenticated()) {
PolicyResult.Accepted(cmd)
@@ -122,14 +139,5 @@ open class FullAuthPolicy(
PolicyResult.Rejected("auth-required: this relay requires authentication")
}
override fun onAuthenticationFailed(pubKey: HexKey) {
// Only undo a brand-new authentication this AUTH added; never drop a
// pubkey that was already authenticated before this attempt.
if (pendingNewlyAdded == pubKey) {
authenticatedUsers.remove(pubKey)
}
pendingNewlyAdded = null
}
override fun canSendToSession(event: Event) = true
}
quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/policies/PolicyStack.kt
-4
View File
@@ -56,10 +56,6 @@ class PolicyStack(
policies.forEach { it.onAuthenticated(pubKey, event) }
}
override fun onAuthenticationFailed(pubKey: HexKey) {
policies.forEach { it.onAuthenticationFailed(pubKey) }
}
override fun acceptMessage(message: String): String? {
for (policy in policies) {
policy.acceptMessage(message)?.let { return it }
quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/NostrServerAuthTest.kt
+5 -5
View File
@@ -538,12 +538,12 @@ class NostrServerAuthTest {
// -- NIP-42: onAuthenticated suspend hook ----------------------------------
@Test
fun onAuthenticatedHookRunsAfterSuccessfulAuth() =
fun authorizeHookRunsAfterSuccessfulAuth() =
runTest {
var hookPubkey: String? = null
val policy =
object : FullAuthPolicy(relayUrl) {
override suspend fun onAuthenticated(
override suspend fun authorize(
pubKey: String,
event: RelayAuthEvent,
) {
@@ -568,11 +568,11 @@ class NostrServerAuthTest {
}
@Test
fun onAuthenticatedThrowTurnsAuthIntoFailingOk() =
fun authorizeThrowTurnsAuthIntoFailingOk() =
runTest {
val policy =
object : FullAuthPolicy(relayUrl) {
override suspend fun onAuthenticated(
override suspend fun authorize(
pubKey: String,
event: RelayAuthEvent,
): Unit = throw IllegalStateException("backend rejected user")
@@ -604,7 +604,7 @@ class NostrServerAuthTest {
runTest {
val policy =
object : FullAuthPolicy(relayUrl) {
override suspend fun onAuthenticated(
override suspend fun authorize(
pubKey: String,
event: RelayAuthEvent,
): Unit = throw IllegalStateException("backend rejected user")