From c89b31aaf1047bc3a6484b62ea69b3e8c8de271b Mon Sep 17 00:00:00 2001 From: Avi Date: Sun, 27 Sep 2026 21:01:23 -0500 Subject: [PATCH] =?UTF-8?q?feat(nip46):=20pair=20a=20second=20signer=20acc?= =?UTF-8?q?ount=20=E2=80=94=20park=20the=20live=20session,=20switch=20re-d?= =?UTF-8?q?ials=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One live NIP-46 session, many saved ones (Option A): - start_pairing/connect while a session is live PARKS it instead of refusing: row, pairing secret, and persisted client key stay intact, so the parked account is restorable with no fresh scan. - SelectProfile follows the switch: target has a restorable connection -> park current + re-dial target's row (expected_identity guard applies); target is local-key or unpaired -> live session untouched. - New nip46_cancel_pairing IPC: aborts ONLY an in-flight pairing and re-dials the parked session, so cancel-after-park is transparent. The QR cancel paths (Add-profile modal, Signer Mode screen) use it — plain disconnect would revoke the parked connection. - e2e: two fake Ambers on one relay; A pairs, B's pairing parks A (revoked_at none, client key resolvable), switch back re-dials A and signs; no-op switch; local profile leaves session alone; B restorable. --- frontend/src/lib/api.ts | 1 + frontend/src/screens/CreateProfileModal.tsx | 10 +- frontend/src/screens/SignerModeScreen.tsx | 11 +- frontend/src/state/AppProvider.tsx | 4 + frontend/src/test/CreateProfileModal.test.tsx | 5 +- frontend/src/test/fakeBackend.ts | 8 + src/ipc.rs | 79 ++++-- src/signer/nip46_client.rs | 142 ++++++++++ tests/nip46_e2e.rs | 245 ++++++++++++++++++ 9 files changed, 482 insertions(+), 23 deletions(-) diff --git a/frontend/src/lib/api.ts b/frontend/src/lib/api.ts index 7d6fdb5..fc67375 100644 --- a/frontend/src/lib/api.ts +++ b/frontend/src/lib/api.ts @@ -121,6 +121,7 @@ export const api = { call('nip46_connect', { uri, label }), nip46PairStart: (label: string) => call('nip46_pair_start', { label }), nip46Disconnect: () => call('nip46_disconnect'), + nip46CancelPairing: () => call('nip46_cancel_pairing'), nip46Status: () => call('nip46_status'), nip46Approve: (id: string, approved: boolean, always = false) => call('nip46_approve', { id, approved, always }), diff --git a/frontend/src/screens/CreateProfileModal.tsx b/frontend/src/screens/CreateProfileModal.tsx index 7c259e5..db39ef7 100644 --- a/frontend/src/screens/CreateProfileModal.tsx +++ b/frontend/src/screens/CreateProfileModal.tsx @@ -15,7 +15,8 @@ interface CreateProfileModalProps { type Phase = 'choice' | 'pairing' | 'paired' | 'local' | 'creating' | 'success'; export function CreateProfileModal({ open, onClose }: CreateProfileModalProps) { - const { state, createProfile, nip46PairStart, nip46Status, nip46Disconnect, refresh } = useApp(); + const { state, createProfile, nip46PairStart, nip46Status, nip46CancelPairing, refresh } = + useApp(); const [label, setLabel] = useState(''); const [phase, setPhase] = useState('choice'); const [error, setError] = useState(null); @@ -149,13 +150,16 @@ export function CreateProfileModal({ open, onClose }: CreateProfileModalProps) { // Leaving the QR view mid-pairing aborts the in-flight pairing; nothing // was persisted yet, so teardown is safe at any point (same as Signer - // Mode's "Cancel pairing"). + // Mode's "Cancel pairing"). Cancel (not disconnect): when pairing a + // second signer account parked the first one, cancelling must restore + // the parked session rather than revoke anything. const cancelPairing = async () => { setLivePairingUri(null); setPairingQr(null); setPhase('choice'); try { - await nip46Disconnect(); + await nip46CancelPairing(); + void refresh(); } catch { // Best-effort abort; a dead pairing attempt expires on its own. } diff --git a/frontend/src/screens/SignerModeScreen.tsx b/frontend/src/screens/SignerModeScreen.tsx index 2bdd00f..83dd1c7 100644 --- a/frontend/src/screens/SignerModeScreen.tsx +++ b/frontend/src/screens/SignerModeScreen.tsx @@ -17,6 +17,7 @@ export function SignerModeScreen() { nip46Connect, nip46PairStart, nip46Disconnect, + nip46CancelPairing, nip46Approve, embeddedSignerApprove, refresh, @@ -192,17 +193,19 @@ export function SignerModeScreen() { }; }, [pairingUri]); - // Abort an in-flight pairing (e.g. expired QR) — same teardown as a - // disconnect; nothing was persisted yet so it is safe at any point. + // Abort an in-flight pairing (e.g. expired QR): cancel the pairing + // attempt only. Never disconnect here — if pairing a second signer + // account parked the first one, a cancel must bring the parked session + // back instead of revoking it. const handlePairCancel = useCallback(async () => { setPairError(null); try { - const status = await nip46Disconnect(); + const status = await nip46CancelPairing(); setNip46StatusState(status); } catch (err) { setPairError(err instanceof Error ? err.message : String(err)); } - }, [nip46Disconnect]); + }, [nip46CancelPairing]); const handleNip46Disconnect = useCallback(async () => { setError(null); diff --git a/frontend/src/state/AppProvider.tsx b/frontend/src/state/AppProvider.tsx index 543620c..9aa5483 100644 --- a/frontend/src/state/AppProvider.tsx +++ b/frontend/src/state/AppProvider.tsx @@ -82,6 +82,7 @@ interface AppContextValue { nip46Connect: (uri: string, label: string) => Promise; nip46PairStart: (label: string) => Promise; nip46Disconnect: () => Promise; + nip46CancelPairing: () => Promise; nip46Status: () => Promise; nip46Approve: (id: string, approved: boolean, always?: boolean) => Promise; // Legacy NIP-46 bunker (deprecated) @@ -289,6 +290,7 @@ export function AppProvider({ children }: { children: ReactNode }) { [], ); const nip46Disconnect = useCallback(() => api.nip46Disconnect(), []); + const nip46CancelPairing = useCallback(() => api.nip46CancelPairing(), []); const nip46Status = useCallback(() => api.nip46Status(), []); const nip46PairStart = useCallback((label: string) => api.nip46PairStart(label), []); const nip46Approve = useCallback( @@ -385,6 +387,7 @@ export function AppProvider({ children }: { children: ReactNode }) { nip46Connect, nip46PairStart, nip46Disconnect, + nip46CancelPairing, nip46Status, nip46Approve, signerConnect, @@ -445,6 +448,7 @@ export function AppProvider({ children }: { children: ReactNode }) { nip46Connect, nip46PairStart, nip46Disconnect, + nip46CancelPairing, nip46Status, nip46Approve, signerConnect, diff --git a/frontend/src/test/CreateProfileModal.test.tsx b/frontend/src/test/CreateProfileModal.test.tsx index da456cb..a4d3123 100644 --- a/frontend/src/test/CreateProfileModal.test.tsx +++ b/frontend/src/test/CreateProfileModal.test.tsx @@ -137,7 +137,10 @@ describe('CreateProfileModal', () => { await screen.findByText(/Waiting for the signer to scan/i); await user.click(screen.getByRole('button', { name: 'Cancel pairing' })); - expect(backend.requests.some((r) => r.method === 'nip46_disconnect')).toBe(true); + // Cancel must be the NON-revoking cancel (parked sessions survive it), + // never the disconnect that revokes the stored connection. + expect(backend.requests.some((r) => r.method === 'nip46_cancel_pairing')).toBe(true); + expect(backend.requests.some((r) => r.method === 'nip46_disconnect')).toBe(false); expect( screen.getByRole('button', { name: /Sign in with a signer app \(Amber\)/ }), ).toBeInTheDocument(); diff --git a/frontend/src/test/fakeBackend.ts b/frontend/src/test/fakeBackend.ts index 47e6954..40b147c 100644 --- a/frontend/src/test/fakeBackend.ts +++ b/frontend/src/test/fakeBackend.ts @@ -226,6 +226,14 @@ export function createFakeBackend(initial?: AppState): FakeBackend { backend.setNip46(next); return next; } + case 'nip46_cancel_pairing': { + // Mirrors the real backend: aborts ONLY the pairing attempt and + // re-dials the parked session — a cancel must not clear a + // connected session, only the pairing URI. + const next = { ...backend.nip46, pairing_uri: undefined }; + backend.setNip46(next); + return next; + } case 'nip46_approve': return backend.nip46; diff --git a/src/ipc.rs b/src/ipc.rs index 5ed59d5..93f8330 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -173,6 +173,12 @@ pub enum Request { }, /// Disconnect from the NIP-46 signer. Nip46Disconnect, + /// Cancel an in-flight pairing (the GUI left the QR view): abort the + /// pairing attempt WITHOUT touching any parked/saved session, then try + /// to re-dial the active profile's saved session so parking for a + /// cancelled pairing is fully transparent. (Plain Nip46Disconnect would + /// revoke the currently stored connection — wrong for a cancel.) + Nip46CancelPairing, /// Get NIP-46 connection status. Nip46Status, /// Approve/reject a pending NIP-46 request. `always = true` additionally @@ -492,6 +498,13 @@ async fn run(app: &Arc>, request: Request) -> Result>, request: Request) -> Result>, request: Request) -> Result { + let Some(signer) = ensure_nip46_signer(app).await else { + return Err(AppError::config("NIP-46 signer not initialized")); + }; + signer.cancel_pairing().await?; + let status = signer.status().await; + Ok(json!(status)) + } Request::Nip46Status => { let Some(signer) = ensure_nip46_signer(app).await else { return Ok(json!({ "connected": false, "error": "Not initialized" })); @@ -654,6 +681,41 @@ async fn run(app: &Arc>, request: Request) -> Result { + { + let mut guard = app.lock().await; + profiles::set_active(&mut guard.vault, &npub)?; + if guard.signer_mode == SignerMode::Embedded { + if let Some(signer) = &guard.embedded_signer { + signer.set_active_profile(Some(npub.clone())).await; + } + } else if guard.signer_mode == SignerMode::Nip46Client { + if let Some(signer) = &guard.nip46_signer { + signer.set_active_profile(Some(npub.clone())).await; + } + } + guard.save_vault()?; + } + // Option A: follow the switch with the signer session. If the + // target profile has a restorable NIP-46 connection, the live + // session (if any, serving a different account) is parked and + // this profile's session is re-dialed — no fresh scan. If the + // target has no signer connection (local-key profile), the live + // session is left alone. + if app.lock().await.signer_mode == SignerMode::Nip46Client { + if let Some(signer) = ensure_nip46_signer(app).await { + if let Err(e) = signer.switch_to_profile(&npub).await { + eprintln!("[NIP46] profile switch session change failed: {e}"); + } + } + } + let guard = app.lock().await; + Ok(json!(guard.state_view())) + } + // Vault state requests (require lock) other => { let mut guard = app.lock().await; @@ -711,21 +773,8 @@ async fn run_with_app(app: &mut App, request: Request) -> Result { - profiles::set_active(&mut app.vault, &npub)?; - if app.signer_mode == SignerMode::Embedded { - if let Some(signer) = &app.embedded_signer { - signer.set_active_profile(Some(npub)).await; - } - } else if app.signer_mode == SignerMode::Nip46Client { - if let Some(signer) = &app.nip46_signer { - signer.set_active_profile(Some(npub)).await; - } - } - app.save_vault()?; - Ok(json!(app.state_view())) - } - + // NOTE: SelectProfile is handled in `run` (above), not here — it + // must drop the App guard before switching the signer session. Request::PublishProfileMetadata { npub } => { // Route through the profile's Signing source, exactly like // PublishNote: an embedded profile signs locally, a paired diff --git a/src/signer/nip46_client.rs b/src/signer/nip46_client.rs index 0d6ab3d..1248714 100644 --- a/src/signer/nip46_client.rs +++ b/src/signer/nip46_client.rs @@ -488,6 +488,148 @@ impl Nip46ClientSigner { self.disconnect().await } + /// Park the live session: stop the wire task and clear in-memory state, + /// but keep the vault row, pairing secret, and persisted client key + /// INTACT, so the session can be re-dialed later with no fresh scan. + /// + /// Unlike [`Self::disconnect`] this does NOT revoke: switching signer + /// accounts (pair a second Amber account, or switch back to a previously + /// paired one) must leave the parked account restorable. The vault row + /// is what `reactivate_saved_sessions` dials from, so a parked session + /// is exactly a saved session. + pub async fn park_live_session(&self) { + let mut inner = self.inner.lock().await; + if let Some(task) = inner.task.take() { + task.abort(); + } + if let Some(pairing) = inner.pairing.take() { + pairing.task.abort(); + } + if let Some(client) = inner.client.take() { + let _ = client.disconnect().await; + } + inner.phase = Nip46Phase::Stopped; + inner.connection = None; + inner.conversation_key = None; + inner.keys = None; + inner.identity = None; + inner.expected_identity = None; + inner.active_npub = None; + inner.pending.clear(); + // Wake any callers awaiting a remote response; their waiters turn + // into `NotConnected` rather than hanging until the request timeout. + for (_, waiter) in inner.remote_pending.drain() { + let _ = waiter.sender.send(Err( + "Switched signer accounts before the signer responded.".to_string(), + )); + } + } + + /// Whether a session or pairing is currently live. + pub async fn has_live_session(&self) -> bool { + let inner = self.inner.lock().await; + inner.task.is_some() || inner.pairing.is_some() + } + + /// Cancel an in-flight pairing attempt: abort ONLY the pairing task and + /// its session state, then try to re-dial the active profile's saved + /// session. Used by the GUI when the user leaves the QR view after + /// pairing-for-a-second-account parked the first one: the cancel must + /// not revoke anything, and the parked session should come back so the + /// park is invisible. + pub async fn cancel_pairing(&self) -> Result<(), AppError> { + { + let mut inner = self.inner.lock().await; + if let Some(pairing) = inner.pairing.take() { + pairing.task.abort(); + } + // A pairing that was cancelled before anyone scanned never + // reached the vault; the only live slot it held is now free. + // If a *connected* session exists (task, not pairing), leave it + // alone — cancelling pairing must not kill a working session. + if inner.task.is_some() { + return Ok(()); + } + inner.phase = Nip46Phase::Stopped; + inner.connection = None; + inner.conversation_key = None; + inner.keys = None; + inner.identity = None; + inner.expected_identity = None; + inner.active_npub = None; + inner.pending.clear(); + } + // The park-then-pair flow (Option A) may have left the previous + // account parked: re-dial it (reactivate prefers the active + // profile's row) so the cancelled pairing changes nothing. + self.reactivate_saved_sessions().await?; + Ok(()) + } + + /// Follow a profile switch with the signer session (Option A: one live + /// session, many saved ones). + /// + /// - The live session already serves `npub`: keep it, do nothing. + /// - `npub` has no live saved NIP-46 connection (local-key profile, or + /// never paired): leave the live session alone — signing for `npub` + /// routes through its own source and killing a working Amber session + /// to look at a local profile would be a regression. + /// - `npub` has a restorable saved connection: park the live session + /// (restorable, not revoked) and re-dial `npub`'s row. + /// + /// Returns `true` when a re-dial was started. The re-dial resolves + /// asynchronously through the same handshake as restore, including the + /// `expected_identity` cross-account guard. + pub async fn switch_to_profile(&self, npub: &str) -> Result { + // Already serving the target identity? Nothing to do. + { + let inner = self.inner.lock().await; + if let Some(identity) = &inner.identity { + if identity.to_bech32().ok().as_deref() == Some(npub) { + return Ok(false); + } + } + } + + // Does the target profile have a live, restorable saved connection? + let restorable = { + let app = self.app.lock().await; + let vault_key = app.vault_key().copied(); + let now = crate::vault::unix_timestamp().unwrap_or(0); + app.vault.nip46_connections.iter().any(|c| { + c.profile_npub.as_deref() == Some(npub) + && c.revoked_at.is_none() + && c.expires_at.map(|t| t > now).unwrap_or(true) + && crate::vault::resolve_connection_client_key( + &app.vault, + vault_key.as_ref(), + &crate::signer::VaultRef::from_connection(c), + ) + .ok() + .flatten() + .is_some() + }) + }; + if !restorable { + return Ok(false); + } + + // Make sure the vault agrees on the target before re-dialing (the + // IPC path already did this; idempotent here, and it makes the + // reactivate preference order correct regardless of caller). + { + let mut app = self.app.lock().await; + if app.vault.active_profile.as_deref() != Some(npub) { + crate::profiles::set_active(&mut app.vault, npub)?; + app.save_vault()?; + } + } + + self.park_live_session().await; + let dialed = self.reactivate_saved_sessions().await?; + Ok(dialed > 0) + } + /// Re-dial the saved signer sessions after startup/unlock, no scan. /// /// Amber remembers our *client pubkey* as the identity of an approved diff --git a/tests/nip46_e2e.rs b/tests/nip46_e2e.rs index 86b281b..0737396 100644 --- a/tests/nip46_e2e.rs +++ b/tests/nip46_e2e.rs @@ -1324,3 +1324,248 @@ async fn nip46_session_restore_redials_and_refuses_wrong_identity() { tokio::time::sleep(Duration::from_millis(100)).await; } } + +// --------------------------------------------------------------------------- +// Option A: many saved sessions, one live. Pairing/connecting a second +// signer account PARKS the live session (restorable, never revoked), and +// switching a profile back to a parked account re-dials it with no scan. +// --------------------------------------------------------------------------- + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +#[allow(clippy::await_holding_lock)] +async fn nip46_second_account_parks_first_and_switch_restores_it() { + let _vault_guard = VAULT_ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let app = { + let tmp = std::env::temp_dir().join(format!( + "keynectr-e2e-switch-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let app_dir = tmp.join("keynectr"); + std::fs::create_dir_all(&app_dir).unwrap(); + std::fs::write( + app_dir.join("profiles_vault.json"), + serde_json::to_string(&Vault::empty()).unwrap(), + ) + .unwrap(); + std::env::set_var("XDG_DATA_HOME", &tmp); + let app = std::sync::Arc::new(Mutex::new(App::load().expect("load app"))); + assert!( + app.try_lock().unwrap().vault.profiles.is_empty(), + "e2e vault isolation failed for switch test" + ); + app + }; + + let relay_url = start_relay().await; + let comms_a = Keys::generate(); + let identity_a = Keys::generate(); + let comms_b = Keys::generate(); + let identity_b = Keys::generate(); + // Two fake Ambers on one relay: each only answers traffic it can + // NIP-44-decrypt with its own comms key, so they never cross-talk. + tokio::spawn(run_fake_amber( + relay_url.clone(), + comms_a.clone(), + identity_a.clone(), + Duration::from_millis(30), + )); + tokio::spawn(run_fake_amber( + relay_url.clone(), + comms_b.clone(), + identity_b.clone(), + Duration::from_millis(30), + )); + + let signer = Nip46ClientSigner::new(app.clone()); + + let wait_connected = |signer: Nip46ClientSigner| async move { + let deadline = tokio::time::Instant::now() + Duration::from_secs(15); + loop { + let st = signer.status().await; + if let Some(err) = &st.error { + panic!("session failed: {err}"); + } + if st.connected { + return; + } + assert!( + tokio::time::Instant::now() < deadline, + "session never connected: {:?}", + signer.status().await + ); + tokio::time::sleep(Duration::from_millis(100)).await; + } + }; + + // --- 1. Account A pairs (the flow the GUI runs). + signer + .connect( + &format!( + "bunker://{}?relay={}", + comms_a.public_key().to_hex(), + relay_url + ), + "amber A".to_string(), + ) + .await + .expect("connect account A"); + wait_connected(signer.clone()).await; + let npub_a = SignerTrait::get_public_key(&signer) + .await + .expect("identity A") + .to_bech32() + .unwrap(); + + // --- 2. Account B pairs while A is live. The IPC dispatcher parks the + // live session first (the exact sequence the dispatcher now runs). + assert!(signer.has_live_session().await); + signer.park_live_session().await; + assert!( + !signer.has_live_session().await, + "park must clear the live slot" + ); + signer + .connect( + &format!( + "bunker://{}?relay={}", + comms_b.public_key().to_hex(), + relay_url + ), + "amber B".to_string(), + ) + .await + .expect("connect account B while A is parked"); + wait_connected(signer.clone()).await; + let npub_b = SignerTrait::get_public_key(&signer) + .await + .expect("identity B") + .to_bech32() + .unwrap(); + assert_ne!(npub_a, npub_b, "two accounts, two identities"); + + // B is fully usable: sign through it. + let unsigned = UnsignedEvent::new( + identity_b.public_key(), + Timestamp::now(), + Kind::TextNote, + vec![], + "signed by B".to_string(), + ); + let signed = SignerTrait::sign_event(&signer, unsigned.clone()) + .await + .expect("sign through B"); + assert!(signed.verify_signature()); + + // Parking must NOT have revoked A: its row stays live with its client + // key resolvable — that is what makes it restorable. + let ref_a = + keynectr::signer::VaultRef::new(Some(npub_a.clone()), comms_a.public_key().to_hex()); + { + let g = app.lock().await; + let row = g + .vault + .nip46_connections + .iter() + .find(|c| c.profile_npub.as_deref() == Some(npub_a.as_str())) + .expect("A's connection row survives B's pairing"); + assert!( + row.revoked_at.is_none(), + "parked session must not be revoked" + ); + assert!( + keynectr::vault::resolve_connection_client_key(&g.vault, None, &ref_a) + .expect("resolve") + .is_some(), + "parked session must keep its client key" + ); + } + + // --- 3. Switch back to A: park B, re-dial A — no fresh pairing. + let dialed = signer + .switch_to_profile(&npub_a) + .await + .expect("switch to A"); + assert!(dialed, "A has a restorable session, switch must re-dial it"); + wait_connected(signer.clone()).await; + let back = SignerTrait::get_public_key(&signer) + .await + .expect("identity after switch"); + assert_eq!( + back.to_bech32().unwrap(), + npub_a, + "switched session must answer as A" + ); + let unsigned_a = UnsignedEvent::new( + identity_a.public_key(), + Timestamp::now(), + Kind::TextNote, + vec![], + "signed by A again".to_string(), + ); + let signed_a = SignerTrait::sign_event(&signer, unsigned_a.clone()) + .await + .expect("sign through A after switch"); + assert!(signed_a.verify_signature()); + assert_eq!(signed_a.content, "signed by A again"); + + // Switching to A again is a no-op (already serving A): no second dial. + assert!( + !signer + .switch_to_profile(&npub_a) + .await + .expect("noop switch"), + "switch to the identity already live must not re-dial" + ); + + // --- 4. A profile with NO signer connection must leave B... (here A) + // alone: local-key profiles route signing through their own source. + let local = Keys::generate(); + let npub_local = local.public_key().to_bech32().unwrap(); + { + let mut g = app.lock().await; + g.vault.profiles.push(keynectr::vault::StoredProfile { + label: "local".to_string(), + public_key: npub_local.clone(), + secret_key: local + .secret_key() + .to_secret_bytes() + .iter() + .map(|b| format!("{b:02x}")) + .collect(), + created_at: 0, + picture: None, + nip05: None, + signer_mode: keynectr::vault::SignerMode::Embedded, + }); + g.save_vault().unwrap(); + } + assert!( + !signer + .switch_to_profile(&npub_local) + .await + .expect("switch to local"), + "local-key profile must not touch the live signer session" + ); + assert!( + signer.status().await.connected, + "live A session survives a switch to a local profile" + ); + let still_a = SignerTrait::get_public_key(&signer).await.expect("still A"); + assert_eq!(still_a.to_bech32().unwrap(), npub_a); + + // And switching back to B works too — B was parked, never revoked. + let dialed_b = signer + .switch_to_profile(&npub_b) + .await + .expect("switch back to B"); + assert!(dialed_b, "B was parked, must be restorable"); + wait_connected(signer.clone()).await; + let b_again = SignerTrait::get_public_key(&signer).await.expect("B again"); + assert_eq!(b_again.to_bech32().unwrap(), npub_b); + + signer.disconnect().await.ok(); +}