diff --git a/quartz/RELAY.md b/quartz/RELAY.md index 93108b469c..8d83811b6c 100644 --- a/quartz/RELAY.md +++ b/quartz/RELAY.md @@ -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) } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/IRelayPolicy.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/IRelayPolicy.kt index 0e8bd170f8..92b709e451 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/IRelayPolicy.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/IRelayPolicy.kt @@ -70,20 +70,19 @@ interface IRelayPolicy { fun accept(cmd: AuthCmd): PolicyResult /** - * 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 diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/RelaySession.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/RelaySession.kt index 8ff6750a32..f480ef4b6e 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/RelaySession.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/RelaySession.kt @@ -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 } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/policies/FullAuthPolicy.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/policies/FullAuthPolicy.kt index 6602160b14..7c7b1a19da 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/policies/FullAuthPolicy.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/policies/FullAuthPolicy.kt @@ -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() - /** - * 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 { 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 = 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 } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/policies/PolicyStack.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/policies/PolicyStack.kt index b9628aa26d..c0654a4ffd 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/policies/PolicyStack.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/policies/PolicyStack.kt @@ -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 } diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/NostrServerAuthTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/NostrServerAuthTest.kt index 534d9804aa..e9cf093320 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/NostrServerAuthTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/server/NostrServerAuthTest.kt @@ -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")