From 859ccca82631d6c59c4632c9b56d93b298710787 Mon Sep 17 00:00:00 2001 From: Yuki Kishimoto Date: Thu, 31 Oct 2024 16:29:49 +0100 Subject: [PATCH] connect: fix `NostrConnect` according to NIP46 Remote signer public key may be different from the user public key so adj. the code to handle them correctly. Closes https://github.com/rust-nostr/nostr/issues/597 Signed-off-by: Yuki Kishimoto --- CHANGELOG.md | 2 +- bindings/nostr-sdk-ffi/src/connect.rs | 5 - bindings/nostr-sdk-js/src/connect.rs | 14 +-- crates/nostr-connect/src/client.rs | 123 ++++++++++++--------- crates/nostr-connect/src/error.rs | 6 +- crates/nostr-connect/src/signer.rs | 2 +- crates/nostr-sdk/examples/nostr-connect.rs | 18 +-- crates/nostr/src/nips/nip46.rs | 39 ++++--- 8 files changed, 113 insertions(+), 96 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29efc0b2b..6df53acb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -123,10 +123,10 @@ ### Fixed * nostr: adj. `NostrConnectURI` de/serialization according to NIP46 ([Yuki Kishimoto]) +* connect: fix `NostrConnect` according to NIP46 * lmdb: add missing commit method call in `Store::delete` ([Yuki Kishimoto]) * lmdb: fix unit tests ([Yuki Kishimoto]) * lmdb: fix `Store::save_event` issues ([Yuki Kishimoto]) -* signer: fix `Nip46Signer` bootstrap on demand when using `bunker` URI ([Yuki Kishimoto]) * sdk: fix `filters empty` error when gossip option is enabled ([Yuki Kishimoto]) ### Removed diff --git a/bindings/nostr-sdk-ffi/src/connect.rs b/bindings/nostr-sdk-ffi/src/connect.rs index 33cb34a0f..0df0647b9 100644 --- a/bindings/nostr-sdk-ffi/src/connect.rs +++ b/bindings/nostr-sdk-ffi/src/connect.rs @@ -65,11 +65,6 @@ impl NostrConnect { .collect() } - /// Get signer public key - pub async fn signer_public_key(&self) -> Result { - Ok(self.inner.signer_public_key().await.copied()?.into()) - } - /// Get `bunker` URI pub async fn bunker_uri(&self) -> Result { Ok(self.inner.bunker_uri().await?.into()) diff --git a/bindings/nostr-sdk-js/src/connect.rs b/bindings/nostr-sdk-js/src/connect.rs index fbf56b6a7..274bb36ac 100644 --- a/bindings/nostr-sdk-js/src/connect.rs +++ b/bindings/nostr-sdk-js/src/connect.rs @@ -10,7 +10,7 @@ use wasm_bindgen::prelude::*; use crate::duration::JsDuration; use crate::error::{into_err, Result}; -use crate::protocol::key::{JsKeys, JsPublicKey}; +use crate::protocol::key::JsKeys; use crate::protocol::nips::nip46::JsNostrConnectURI; use crate::JsStringArray; @@ -64,18 +64,6 @@ impl JsNostrConnect { .unchecked_into() } - /// Get signer public key - #[wasm_bindgen(js_name = signerPublicKey)] - pub async fn signer_public_key(&self) -> Result { - Ok(self - .inner - .signer_public_key() - .await - .copied() - .map_err(into_err)? - .into()) - } - /// Get `bunker` URI #[wasm_bindgen(js_name = bunkerUri)] pub async fn bunker_uri(&self) -> Result { diff --git a/crates/nostr-connect/src/client.rs b/crates/nostr-connect/src/client.rs index dbe018b4f..cf049c172 100644 --- a/crates/nostr-connect/src/client.rs +++ b/crates/nostr-connect/src/client.rs @@ -5,8 +5,6 @@ //! Nostr Connect client use std::collections::HashMap; -use std::sync::atomic::{AtomicBool, Ordering}; -use std::sync::Arc; use std::time::Duration; use async_trait::async_trait; @@ -30,14 +28,14 @@ pub type Nip46Signer = NostrConnect; /// #[derive(Debug, Clone)] pub struct NostrConnect { - app_keys: Keys, uri: NostrConnectURI, - signer_public_key: OnceCell, + app_keys: Keys, + remote_signer_public_key: OnceCell, + user_public_key: OnceCell, pool: RelayPool, timeout: Duration, opts: RelayOptions, secret: Option, - bootstrapped: Arc, } impl NostrConnect { @@ -55,21 +53,17 @@ impl NostrConnect { } } - // Get signer public key - let signer_public_key: OnceCell = match uri.signer_public_key() { - Some(public_key) => OnceCell::from(public_key), - None => OnceCell::new(), - }; - Ok(Self { app_keys, - signer_public_key, + // NOT set the remote_signer_public_key, also if bunker URI! + // If you already set remote_signer_public_key, you'll need another field to know if boostrap was already done. + remote_signer_public_key: OnceCell::new(), + user_public_key: OnceCell::new(), pool: RelayPool::default(), timeout, opts: opts.unwrap_or_default(), secret: uri.secret(), uri, - bootstrapped: Arc::new(AtomicBool::new(false)), }) } @@ -85,20 +79,31 @@ impl NostrConnect { // Subscribe let notifications = self.subscribe().await?; - // Get signer public key - let signer_public_key: PublicKey = match self.uri.signer_public_key() { - Some(public_key) => public_key, - None => get_signer_public_key(&self.app_keys, notifications, self.timeout).await?, + // Get remote signer public key + let remote_signer_public_key: PublicKey = match self.uri.remote_signer_public_key() { + Some(public_key) => *public_key, + None => { + match get_remote_signer_public_key(&self.app_keys, notifications, self.timeout) + .await? + { + GetRemoteSignerPublicKey::RemoteOnly(public_key) => public_key, + GetRemoteSignerPublicKey::WithUserPublicKey { remote, user } => { + // Set user public key + self.user_public_key.set(user)?; // This shouldn't fails + + // Return remote signer public key + remote + } + } + } }; // Send `connect` command if bunker URI if self.uri.is_bunker() { - self.connect(signer_public_key).await?; + self.connect(remote_signer_public_key).await?; } - self.bootstrapped.store(true, Ordering::SeqCst); - - Ok(signer_public_key) + Ok(remote_signer_public_key) } async fn subscribe(&self) -> Result, Error> { @@ -131,43 +136,36 @@ impl NostrConnect { self.uri.relays() } - /// Get signer [PublicKey] - #[inline] - pub async fn signer_public_key(&self) -> Result<&PublicKey, Error> { - // The bootstrap here is executed only if URI is NOT `bunker://` - self.signer_public_key - .get_or_try_init(|| async { self.bootstrap().await }) - .await - } - /// Get `bunker` URI pub async fn bunker_uri(&self) -> Result { Ok(NostrConnectURI::Bunker { - signer_public_key: *self.signer_public_key().await?, + remote_signer_public_key: *self.remote_signer_public_key().await?, relays: self.relays(), secret: self.secret.clone(), }) } #[inline] - async fn send_request(&self, req: Request) -> Result { - // Get signer public key - let signer_public_key: PublicKey = *self.signer_public_key().await?; + async fn remote_signer_public_key(&self) -> Result<&PublicKey, Error> { + self.remote_signer_public_key + .get_or_try_init(|| async { self.bootstrap().await }) + .await + } - // Check if bootstrap is executed - // If it's not executed, bootstrap. - if !self.bootstrapped.load(Ordering::SeqCst) { - self.bootstrap().await?; - } + #[inline] + async fn send_request(&self, req: Request) -> Result { + // Get remote signer public key + let remote_signer_public_key: PublicKey = *self.remote_signer_public_key().await?; // Send request - self.send_request_with_pk(req, signer_public_key).await + self.send_request_with_pk(req, remote_signer_public_key) + .await } async fn send_request_with_pk( &self, req: Request, - signer_public_key: PublicKey, + remote_signer_public_key: PublicKey, ) -> Result { let secret_key: &SecretKey = self.app_keys.secret_key(); @@ -176,8 +174,9 @@ impl NostrConnect { tracing::debug!("Sending '{msg}' NIP46 message"); let req_id = msg.id().to_string(); - let event: Event = EventBuilder::nostr_connect(&self.app_keys, signer_public_key, msg)? - .sign_with_keys(&self.app_keys)?; + let event: Event = + EventBuilder::nostr_connect(&self.app_keys, remote_signer_public_key, msg)? + .sign_with_keys(&self.app_keys)?; let mut notifications = self.pool.notifications(); @@ -221,12 +220,14 @@ impl NostrConnect { } /// Connect msg - async fn connect(&self, signer_public_key: PublicKey) -> Result<(), Error> { + async fn connect(&self, remote_signer_public_key: PublicKey) -> Result<(), Error> { let req = Request::Connect { - public_key: signer_public_key, + public_key: remote_signer_public_key, secret: self.secret.clone(), }; - let res = self.send_request_with_pk(req, signer_public_key).await?; + let res = self + .send_request_with_pk(req, remote_signer_public_key) + .await?; Ok(res.to_connect()?) } @@ -237,6 +238,15 @@ impl NostrConnect { Ok(res.to_get_relays()?) } + async fn _get_public_key(&self) -> Result<&PublicKey, Error> { + self.user_public_key + .get_or_try_init(|| async { + let res = self.send_request(Request::GetPublicKey).await?; + Ok(res.to_get_public_key()?) + }) + .await + } + /// Sign an [UnsignedEvent] async fn _sign_event(&self, unsigned: UnsignedEvent) -> Result { let req = Request::SignEvent(unsigned); @@ -302,11 +312,16 @@ impl NostrConnect { } } -async fn get_signer_public_key( +enum GetRemoteSignerPublicKey { + WithUserPublicKey { remote: PublicKey, user: PublicKey }, + RemoteOnly(PublicKey), +} + +async fn get_remote_signer_public_key( app_keys: &Keys, mut notifications: Receiver, timeout: Duration, -) -> Result { +) -> Result { time::timeout(Some(timeout), async { while let Ok(notification) = notifications.recv().await { if let RelayPoolNotification::Event { event, .. } = notification { @@ -326,13 +341,16 @@ async fn get_signer_public_key( req: Request::Connect { public_key, .. }, .. } => { - return Ok(public_key); + return Ok(GetRemoteSignerPublicKey::WithUserPublicKey { + remote: event.pubkey, + user: public_key, + }); } Message::Response { result: Some(ResponseResult::Connect), error: None, .. - } => return Ok(event.pubkey), + } => return Ok(GetRemoteSignerPublicKey::RemoteOnly(event.pubkey)), _ => {} } } @@ -349,11 +367,10 @@ async fn get_signer_public_key( #[cfg_attr(not(target_arch = "wasm32"), async_trait)] impl NostrSigner for NostrConnect { async fn get_public_key(&self) -> Result { - // TODO: avoid copied? - self.signer_public_key() + self._get_public_key() .await - .map_err(SignerError::backend) .copied() + .map_err(SignerError::backend) } async fn sign_event(&self, unsigned: UnsignedEvent) -> Result { diff --git a/crates/nostr-connect/src/error.rs b/crates/nostr-connect/src/error.rs index bb0a32bd1..8a635431f 100644 --- a/crates/nostr-connect/src/error.rs +++ b/crates/nostr-connect/src/error.rs @@ -6,8 +6,9 @@ use nostr::event::builder; use nostr::nips::{nip04, nip46}; -use nostr::{key, serde_json}; +use nostr::{key, serde_json, PublicKey}; use thiserror::Error; +use tokio::sync::SetError; /// Nostr Connect error #[derive(Debug, Error)] @@ -33,6 +34,9 @@ pub enum Error { /// Pool #[error(transparent)] Pool(#[from] nostr_relay_pool::pool::Error), + /// Set user public key error + #[error(transparent)] + SetUserPublicKey(#[from] SetError), /// NIP46 response error #[error("response error: {0}")] Response(String), diff --git a/crates/nostr-connect/src/signer.rs b/crates/nostr-connect/src/signer.rs index 33d341c03..bee7f0287 100644 --- a/crates/nostr-connect/src/signer.rs +++ b/crates/nostr-connect/src/signer.rs @@ -83,7 +83,7 @@ impl NostrConnectRemoteSigner { /// Get `bunker` URI pub async fn bunker_uri(&self) -> NostrConnectURI { NostrConnectURI::Bunker { - signer_public_key: self.keys.public_key(), + remote_signer_public_key: self.keys.public_key(), relays: self.relays().await, secret: self.secret.clone(), } diff --git a/crates/nostr-sdk/examples/nostr-connect.rs b/crates/nostr-sdk/examples/nostr-connect.rs index e419927ae..0adca9cbd 100644 --- a/crates/nostr-sdk/examples/nostr-connect.rs +++ b/crates/nostr-sdk/examples/nostr-connect.rs @@ -14,17 +14,17 @@ async fn main() -> Result<()> { let app_keys = Keys::parse("nsec1j4c6269y9w0q2er2xjw8sv2ehyrtfxq3jwgdlxj6qfn8z4gjsq5qfvfk99")?; // Compose signer from bunker URI - let uri = NostrConnectURI::parse("bunker://79dff8f82963424e0bb02708a22e44b4980893e3a4be0fa3cb60a43b946764e3?relay=wss%3A%2F%2Frelay.nsec.app")?; - let signer = NostrConnect::new(uri, app_keys, Duration::from_secs(60), None)?; + let uri = NostrConnectURI::parse("bunker://79dff8f82963424e0bb02708a22e44b4980893e3a4be0fa3cb60a43b946764e3?relay=wss://relay.nsec.app")?; + let signer = NostrConnect::new(uri, app_keys, Duration::from_secs(120), None)?; // Compose signer - /* let uri = NostrConnectURI::client( - app_keys.public_key(), - [Url::parse("wss://relay.nsec.app")?], - "Test app", - ); - println!("\n{uri}\n"); - let signer = NostrConnect::new(uri, app_keys, Duration::from_secs(60), None).await?; */ + // let uri = NostrConnectURI::client( + // app_keys.public_key(), + // [Url::parse("wss://relay.nsec.app")?], + // "Test app", + // ); + // println!("\n{uri}\n"); + // let signer = NostrConnect::new(uri, app_keys, Duration::from_secs(120), None)?; // Get bunker URI for future connections let bunker_uri: NostrConnectURI = signer.bunker_uri().await?; diff --git a/crates/nostr/src/nips/nip46.rs b/crates/nostr/src/nips/nip46.rs index 1928ca463..3dfd77a9d 100644 --- a/crates/nostr/src/nips/nip46.rs +++ b/crates/nostr/src/nips/nip46.rs @@ -723,8 +723,8 @@ impl JsonUtil for NostrConnectMetadata { pub enum NostrConnectURI { /// Direct connection initiated by remote signer Bunker { - /// Signer public key - signer_public_key: PublicKey, + /// Remote signer public key + remote_signer_public_key: PublicKey, /// List of relays to use relays: Vec, /// Optional secret @@ -786,7 +786,7 @@ impl NostrConnectURI { } return Ok(Self::Bunker { - signer_public_key: public_key, + remote_signer_public_key: public_key, relays, secret, }); @@ -837,12 +837,21 @@ impl NostrConnectURI { } /// Get signer public key, if exists. - #[inline] + #[deprecated(since = "0.36.0", note = "Use `remote_signer_public_key` instead.")] pub fn signer_public_key(&self) -> Option { + self.remote_signer_public_key().copied() + } + + /// Get remote signer public key (exists only for `bunker` URIs) + /// + /// This public key MAY be same as the user one, but not necessarily. + #[inline] + pub fn remote_signer_public_key(&self) -> Option<&PublicKey> { match self { Self::Bunker { - signer_public_key, .. - } => Some(*signer_public_key), + remote_signer_public_key, + .. + } => Some(remote_signer_public_key), Self::Client { .. } => None, } } @@ -879,7 +888,7 @@ impl fmt::Display for NostrConnectURI { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { Self::Bunker { - signer_public_key, + remote_signer_public_key, relays, secret, } => { @@ -907,11 +916,14 @@ impl fmt::Display for NostrConnectURI { } if query.is_empty() { - write!(f, "{NOSTR_CONNECT_BUNKER_URI_SCHEME}://{signer_public_key}") + write!( + f, + "{NOSTR_CONNECT_BUNKER_URI_SCHEME}://{remote_signer_public_key}" + ) } else { write!( f, - "{NOSTR_CONNECT_BUNKER_URI_SCHEME}://{signer_public_key}?{query}" + "{NOSTR_CONNECT_BUNKER_URI_SCHEME}://{remote_signer_public_key}?{query}" ) } } @@ -950,14 +962,15 @@ mod test { let uri = "bunker://79dff8f82963424e0bb02708a22e44b4980893e3a4be0fa3cb60a43b946764e3?relay=wss://relay.nsec.app"; let uri = NostrConnectURI::parse(uri).unwrap(); - let signer_public_key = + let remote_signer_public_key = PublicKey::parse("79dff8f82963424e0bb02708a22e44b4980893e3a4be0fa3cb60a43b946764e3") .unwrap(); let relay_url = Url::parse("wss://relay.nsec.app").unwrap(); + assert_eq!(uri.relays(), vec![relay_url.clone()]); assert_eq!( uri, NostrConnectURI::Bunker { - signer_public_key, + remote_signer_public_key, relays: vec![relay_url], secret: None } @@ -981,13 +994,13 @@ mod test { fn test_bunker_uri_serialization() { let uri = "bunker://79dff8f82963424e0bb02708a22e44b4980893e3a4be0fa3cb60a43b946764e3?relay=wss://relay.nsec.app&secret=abcd"; - let signer_public_key = + let remote_signer_public_key = PublicKey::parse("79dff8f82963424e0bb02708a22e44b4980893e3a4be0fa3cb60a43b946764e3") .unwrap(); let relay_url = Url::parse("wss://relay.nsec.app").unwrap(); assert_eq!( NostrConnectURI::Bunker { - signer_public_key, + remote_signer_public_key, relays: vec![relay_url], secret: Some(String::from("abcd")) }