From fdad9186b8ee2994052a00a68d6abd1e2e91232d Mon Sep 17 00:00:00 2001 From: tidely <43219534+tidely@users.noreply.github.com> Date: Tue, 18 Aug 2026 10:02:20 +0000 Subject: [PATCH] git_ui: Dismiss `askpass` prompts when requests end (#61292) # Objective Fixes #47623 - When authenticating git commands using a security key through `askpass`. The modal which asks for user presence does not get dismissed even after the git command finishes successfully. ## Solution Add a cancellation task to the `AskPassModal`, which gets dropped when the requested operation completes. This cancellation task then dismisses the modal. ## Testing - I've tried authentication through `askpass` using my own security key. Testing both successful and failed authentication. - I've added tests which confirm that the modal gets dismissed when a task is cancelled, and that the cancellation is triggered when `ask_password` Task gets dropped. Willing to pair on review, message me on Slack. Showcase video left out because it would leak private information. ## Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Fixed authentication prompts not dismissing automatically when using security keys with ssh --- Cargo.lock | 2 + crates/askpass/Cargo.toml | 3 + crates/askpass/src/askpass.rs | 75 ++++- crates/git_ui/src/git_panel.rs | 30 +- crates/git_ui_core/src/askpass_modal.rs | 59 +++- crates/git_ui_core/src/worktree_service.rs | 30 +- crates/project/src/git_store.rs | 279 +++++++++++++++--- crates/project_benchmarks/src/main.rs | 1 + crates/remote/src/remote_client.rs | 1 + crates/remote/src/transport/mock.rs | 1 + crates/remote/src/transport/ssh.rs | 12 +- crates/remote_connection/Cargo.toml | 8 +- .../src/remote_connection.rs | 129 +++++++- 13 files changed, 559 insertions(+), 71 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 795928677a4..633b0a3f951 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -15355,6 +15355,7 @@ dependencies = [ "anyhow", "askpass", "auto_update", + "editor", "futures 0.3.32", "gpui", "log", @@ -15364,6 +15365,7 @@ dependencies = [ "remote", "semver", "settings", + "theme", "theme_settings", "ui", "ui_input", diff --git a/crates/askpass/Cargo.toml b/crates/askpass/Cargo.toml index 671ab34df25..9c2f97ba880 100644 --- a/crates/askpass/Cargo.toml +++ b/crates/askpass/Cargo.toml @@ -28,5 +28,8 @@ which.workspace = true [target.'cfg(target_os = "windows")'.dependencies] windows.workspace = true +[dev-dependencies] +gpui = { workspace = true, features = ["test-support"] } + [package.metadata.cargo-machete] ignored = ["log"] diff --git a/crates/askpass/src/askpass.rs b/crates/askpass/src/askpass.rs index 9ace8016bd7..6579538a265 100644 --- a/crates/askpass/src/askpass.rs +++ b/crates/askpass/src/askpass.rs @@ -42,8 +42,14 @@ pub enum AskPassResult { Timedout, } +type PasswordPrompt = ( + String, + oneshot::Sender, + oneshot::Receiver<()>, +); + pub struct AskPassDelegate { - tx: mpsc::UnboundedSender<(String, oneshot::Sender)>, + tx: mpsc::UnboundedSender, executor: BackgroundExecutor, _task: Task<()>, } @@ -56,10 +62,26 @@ impl AskPassDelegate { + Sync + 'static, ) -> Self { - let (tx, mut rx) = mpsc::unbounded::<(String, oneshot::Sender<_>)>(); + Self::new_with_cancellation(cx, move |prompt, response_sender, _cancellation, cx| { + password_prompt(prompt, response_sender, cx) + }) + } + + pub fn new_with_cancellation( + cx: &mut AsyncApp, + password_prompt: impl Fn( + String, + oneshot::Sender, + oneshot::Receiver<()>, + &mut AsyncApp, + ) + Send + + Sync + + 'static, + ) -> Self { + let (tx, mut rx) = mpsc::unbounded::(); let task = cx.spawn(async move |cx: &mut AsyncApp| { - while let Some((prompt, channel)) = rx.next().await { - password_prompt(prompt, channel, cx); + while let Some((prompt, response_sender, cancellation)) = rx.next().await { + password_prompt(prompt, response_sender, cancellation, cx); } }); Self { @@ -69,12 +91,17 @@ impl AskPassDelegate { } } - pub fn ask_password(&mut self, prompt: String) -> Task> { + pub fn ask_password(&self, prompt: String) -> Task> { let mut this_tx = self.tx.clone(); self.executor.spawn(async move { - let (tx, rx) = oneshot::channel(); - this_tx.send((prompt, tx)).await.ok()?; - rx.await.ok() + let (response_sender, response_receiver) = oneshot::channel(); + let (cancellation_sender, cancellation_receiver) = oneshot::channel(); + this_tx + .send((prompt, response_sender, cancellation_receiver)) + .await + .ok()?; + let _cancellation_sender = cancellation_sender; + response_receiver.await.ok() }) } } @@ -98,7 +125,7 @@ impl AskPassSession { /// This will create a new AskPassSession. /// You must retain this session until the master process exits. #[must_use] - pub async fn new(executor: BackgroundExecutor, mut delegate: AskPassDelegate) -> Result { + pub async fn new(executor: BackgroundExecutor, delegate: AskPassDelegate) -> Result { #[cfg(target_os = "windows")] let secret = std::sync::Arc::new(std::sync::Mutex::new(None)); @@ -587,3 +614,33 @@ fn find_gpg_program() -> Option { .into_iter() .find_map(|candidate| which::which(candidate).ok()) } + +#[cfg(test)] +mod tests { + use super::*; + use gpui::TestAppContext; + + #[gpui::test] + async fn dropping_password_request_cancels_prompt(cx: &mut TestAppContext) { + let (prompt_sender, mut prompt_receiver) = mpsc::unbounded(); + let delegate = AskPassDelegate::new_with_cancellation( + &mut cx.to_async(), + move |_, response_sender, cancellation, _| { + prompt_sender + .unbounded_send((response_sender, cancellation)) + .expect("prompt receiver should remain open"); + }, + ); + + let password_request = delegate.ask_password("Password:".to_string()); + let (response_sender, cancellation) = prompt_receiver + .next() + .await + .expect("password prompt should be received"); + + drop(password_request); + + assert!(cancellation.await.is_err()); + drop(response_sender); + } +} diff --git a/crates/git_ui/src/git_panel.rs b/crates/git_ui/src/git_panel.rs index 16ea1c3914e..1b0f50bb7c8 100644 --- a/crates/git_ui/src/git_panel.rs +++ b/crates/git_ui/src/git_panel.rs @@ -4210,17 +4210,27 @@ impl GitPanel { let workspace = self.workspace.clone(); let operation = operation.into(); let window = window.window_handle(); - AskPassDelegate::new(&mut cx.to_async(), move |prompt, tx, cx| { - window - .update(cx, |_, window, cx| { - workspace.update(cx, |workspace, cx| { - workspace.toggle_modal(window, cx, |window, cx| { - AskPassModal::new(operation.clone(), prompt.into(), tx, window, cx) - }); + AskPassDelegate::new_with_cancellation( + &mut cx.to_async(), + move |prompt, tx, cancellation, cx| { + window + .update(cx, |_, window, cx| { + workspace.update(cx, |workspace, cx| { + workspace.toggle_modal(window, cx, |window, cx| { + AskPassModal::new( + operation.clone(), + prompt.into(), + tx, + cancellation, + window, + cx, + ) + }); + }) }) - }) - .ok(); - }) + .ok(); + }, + ) } fn can_push_and_pull(&self, cx: &App) -> bool { diff --git a/crates/git_ui_core/src/askpass_modal.rs b/crates/git_ui_core/src/askpass_modal.rs index 1e63a0d03e3..c34d35dfd6c 100644 --- a/crates/git_ui_core/src/askpass_modal.rs +++ b/crates/git_ui_core/src/askpass_modal.rs @@ -1,7 +1,7 @@ use askpass::EncryptedPassword; use editor::Editor; use futures::channel::oneshot; -use gpui::{AppContext, DismissEvent, Entity, EventEmitter, Focusable, Styled}; +use gpui::{AppContext, DismissEvent, Entity, EventEmitter, Focusable, Styled, Task}; use ui::{ ActiveTheme, AnyElement, App, Button, Clickable, Color, Context, DynamicSpacing, Headline, HeadlineSize, Icon, IconName, IconSize, InteractiveElement, IntoElement, Label, LabelCommon, @@ -17,6 +17,7 @@ pub struct AskPassModal { prompt: SharedString, editor: Entity, tx: Option>, + _cancellation_task: Task<()>, } impl EventEmitter for AskPassModal {} @@ -32,6 +33,7 @@ impl AskPassModal { operation: SharedString, prompt: SharedString, tx: oneshot::Sender, + cancellation: oneshot::Receiver<()>, window: &mut Window, cx: &mut Context, ) -> Self { @@ -44,11 +46,16 @@ impl AskPassModal { } editor }); + let cancellation_task = cx.spawn(async move |this, cx| { + cancellation.await.ok(); + this.update(cx, |_, cx| cx.emit(DismissEvent)).ok(); + }); Self { operation, prompt, editor, tx: Some(tx), + _cancellation_task: cancellation_task, } } @@ -145,3 +152,53 @@ impl Render for AskPassModal { .children(self.render_hint(cx)) } } + +#[cfg(test)] +mod tests { + use std::sync::{Arc, atomic::AtomicBool}; + + use gpui::TestAppContext; + use settings::SettingsStore; + + use super::*; + + #[gpui::test] + fn dismisses_when_password_request_is_cancelled(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + theme_settings::init(theme::LoadThemes::JustBase, cx); + editor::init(cx); + }); + + let (response_sender, _response_receiver) = oneshot::channel(); + let (cancellation_sender, cancellation) = oneshot::channel(); + let window = cx.add_window(|window, cx| Editor::single_line(window, cx)); + let modal = window + .update(cx, |_, window, cx| { + cx.new(|cx| { + AskPassModal::new( + "git fetch".into(), + "Confirm user presence".into(), + response_sender, + cancellation, + window, + cx, + ) + }) + }) + .expect("test window should remain open"); + let dismissed = Arc::new(AtomicBool::new(false)); + let _subscription = cx.update(|cx| { + let dismissed = dismissed.clone(); + cx.subscribe(&modal, move |_, _: &DismissEvent, _| { + dismissed.store(true, std::sync::atomic::Ordering::SeqCst); + }) + }); + + drop(cancellation_sender); + cx.run_until_parked(); + + assert!(dismissed.load(std::sync::atomic::Ordering::SeqCst)); + } +} diff --git a/crates/git_ui_core/src/worktree_service.rs b/crates/git_ui_core/src/worktree_service.rs index fa5c1764ddb..63e9ad08aa5 100644 --- a/crates/git_ui_core/src/worktree_service.rs +++ b/crates/git_ui_core/src/worktree_service.rs @@ -387,17 +387,27 @@ fn create_worktree_askpass_delegate( ) -> AskPassDelegate { let operation = operation.into(); let window = window.window_handle(); - AskPassDelegate::new(&mut cx.to_async(), move |prompt, tx, cx| { - window - .update(cx, |_, window, cx| { - workspace.update(cx, |workspace, cx| { - workspace.toggle_modal(window, cx, |window, cx| { - AskPassModal::new(operation.clone(), prompt.into(), tx, window, cx) - }); + AskPassDelegate::new_with_cancellation( + &mut cx.to_async(), + move |prompt, tx, cancellation, cx| { + window + .update(cx, |_, window, cx| { + workspace.update(cx, |workspace, cx| { + workspace.toggle_modal(window, cx, |window, cx| { + AskPassModal::new( + operation.clone(), + prompt.into(), + tx, + cancellation, + window, + cx, + ) + }); + }) }) - }) - .ok(); - }) + .ok(); + }, + ) } async fn fetch_remote_for_worktree_base( diff --git a/crates/project/src/git_store.rs b/crates/project/src/git_store.rs index 2bd68472059..32656c527cb 100644 --- a/crates/project/src/git_store.rs +++ b/crates/project/src/git_store.rs @@ -614,7 +614,7 @@ pub struct Repository { job_debug_queue: job_debug_queue::GitJobDebugQueue, pending_ops: SumTree, job_id: JobId, - askpass_delegates: Arc>>, + askpass_delegates: RemoteAskPassDelegates, latest_askpass_id: u64, repository_state: Shared>>, initial_graph_data: HashMap<(LogSource, LogOrder), InitialGitGraphData>, @@ -622,6 +622,42 @@ pub struct Repository { commit_data: HashMap, } +type RemoteAskPassDelegates = Arc>>; + +struct RemoteAskPassDelegate { + delegate: AskPassDelegate, + active_request_cancellation: Option>, +} + +struct RemoteAskPassOperation { + askpass_id: u64, + delegates: RemoteAskPassDelegates, +} + +impl RemoteAskPassOperation { + fn new(askpass_id: u64, delegate: AskPassDelegate, delegates: RemoteAskPassDelegates) -> Self { + let previous = delegates.lock().insert( + askpass_id, + RemoteAskPassDelegate { + delegate, + active_request_cancellation: None, + }, + ); + debug_assert!(previous.is_none()); + Self { + askpass_id, + delegates, + } + } +} + +impl Drop for RemoteAskPassOperation { + fn drop(&mut self) { + let delegate = self.delegates.lock().remove(&self.askpass_id); + debug_assert!(delegate.is_some()); + } +} + impl std::ops::Deref for Repository { type Target = RepositorySnapshot; @@ -4469,19 +4505,12 @@ impl GitStore { let repository = Self::repository_for_request(&this, repository_id, &mut cx)?; let delegates = cx.update(|cx| repository.read(cx).askpass_delegates.clone()); - let Some(mut askpass) = delegates.lock().remove(&envelope.payload.askpass_id) else { - debug_panic!("no askpass found"); - anyhow::bail!("no askpass found"); - }; - - let response = askpass - .ask_password(envelope.payload.prompt) - .await - .ok_or_else(|| anyhow::anyhow!("askpass cancelled"))?; - - delegates - .lock() - .insert(envelope.payload.askpass_id, askpass); + let response = request_remote_password( + &delegates, + envelope.payload.askpass_id, + envelope.payload.prompt, + ) + .await?; // In fact, we don't quite know what we're doing here, as we're sending askpass password unencrypted, but.. Ok(proto::AskPassResponse { @@ -5812,6 +5841,41 @@ fn make_remote_delegate( }) } +async fn request_remote_password( + delegates: &RemoteAskPassDelegates, + askpass_id: u64, + prompt: String, +) -> Result { + let (password_request, operation_cancellation) = { + let mut delegates = delegates.lock(); + let delegate = delegates + .get_mut(&askpass_id) + .context("remote Git operation no longer exists")?; + if delegate.active_request_cancellation.is_some() { + bail!("another askpass request is already active"); + } + + let (cancellation_sender, cancellation_receiver) = oneshot::channel(); + let password_request = delegate.delegate.ask_password(prompt); + delegate.active_request_cancellation = Some(cancellation_sender); + (password_request.fuse(), cancellation_receiver.fuse()) + }; + + futures::pin_mut!(password_request, operation_cancellation); + let response = futures::select_biased! { + response = password_request => response, + _ = operation_cancellation => None, + }; + + let mut delegates = delegates.lock(); + let delegate = delegates + .get_mut(&askpass_id) + .context("remote Git operation ended while awaiting askpass")?; + delegate.active_request_cancellation.take(); + + response.context("askpass cancelled") +} + impl RepositoryId { pub fn to_proto(self) -> u64 { self.0 @@ -8301,11 +8365,8 @@ impl Repository { .await } RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => { - askpass_delegates.lock().insert(askpass_id, askpass); - let _defer = util::defer(|| { - let askpass_delegate = askpass_delegates.lock().remove(&askpass_id); - debug_assert!(askpass_delegate.is_some()); - }); + let _askpass_operation = + RemoteAskPassOperation::new(askpass_id, askpass, askpass_delegates); let (name, email) = name_and_email.unzip(); client .request(proto::Commit { @@ -8404,11 +8465,8 @@ impl Repository { result } RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => { - askpass_delegates.lock().insert(askpass_id, askpass); - let _defer = util::defer(|| { - let askpass_delegate = askpass_delegates.lock().remove(&askpass_id); - debug_assert!(askpass_delegate.is_some()); - }); + let _askpass_operation = + RemoteAskPassOperation::new(askpass_id, askpass, askpass_delegates); let response = client .request(proto::Fetch { @@ -8487,11 +8545,8 @@ impl Repository { result } RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => { - askpass_delegates.lock().insert(askpass_id, askpass); - let _defer = util::defer(|| { - let askpass_delegate = askpass_delegates.lock().remove(&askpass_id); - debug_assert!(askpass_delegate.is_some()); - }); + let _askpass_operation = + RemoteAskPassOperation::new(askpass_id, askpass, askpass_delegates); let response = client .request(proto::Push { project_id: project_id.0, @@ -8563,11 +8618,8 @@ impl Repository { .await } RepositoryState::Remote(RemoteRepositoryState { project_id, client }) => { - askpass_delegates.lock().insert(askpass_id, askpass); - let _defer = util::defer(|| { - let askpass_delegate = askpass_delegates.lock().remove(&askpass_id); - debug_assert!(askpass_delegate.is_some()); - }); + let _askpass_operation = + RemoteAskPassOperation::new(askpass_id, askpass, askpass_delegates); let response = client .request(proto::Pull { project_id: project_id.0, @@ -11078,6 +11130,165 @@ mod tests { }); } + type TestPasswordPrompt = ( + String, + oneshot::Sender, + oneshot::Receiver<()>, + ); + + fn test_askpass_delegate( + cx: &mut TestAppContext, + ) -> (AskPassDelegate, mpsc::UnboundedReceiver) { + let (prompt_sender, prompt_receiver) = mpsc::unbounded(); + let delegate = AskPassDelegate::new_with_cancellation( + &mut cx.to_async(), + move |prompt, response_sender, cancellation, _| { + prompt_sender + .unbounded_send((prompt, response_sender, cancellation)) + .expect("prompt receiver should remain open"); + }, + ); + (delegate, prompt_receiver) + } + + #[gpui::test] + async fn ending_remote_operation_cancels_active_askpass(cx: &mut TestAppContext) { + let delegates = RemoteAskPassDelegates::default(); + let (delegate, mut prompts) = test_askpass_delegate(cx); + let operation = RemoteAskPassOperation::new(1, delegate, delegates.clone()); + let password_request = cx.executor().spawn({ + let delegates = delegates.clone(); + async move { request_remote_password(&delegates, 1, "Password:".to_string()).await } + }); + let (_, response_sender, cancellation) = prompts + .next() + .await + .expect("password prompt should be received"); + + drop(operation); + + assert!(cancellation.await.is_err()); + let Err(error) = password_request.await else { + panic!("password request should be cancelled") + }; + assert!(error.to_string().contains("remote Git operation ended")); + assert!(delegates.lock().is_empty()); + drop(response_sender); + } + + #[gpui::test] + async fn successful_remote_askpass_allows_another_prompt(cx: &mut TestAppContext) { + let delegates = RemoteAskPassDelegates::default(); + let (delegate, mut prompts) = test_askpass_delegate(cx); + let operation = RemoteAskPassOperation::new(1, delegate, delegates.clone()); + + for expected_prompt in ["Username:", "Password:"] { + let password_request = + cx.executor().spawn({ + let delegates = delegates.clone(); + async move { + request_remote_password(&delegates, 1, expected_prompt.to_string()).await + } + }); + let (prompt, response_sender, cancellation) = prompts + .next() + .await + .expect("password prompt should be received"); + assert_eq!(prompt, expected_prompt); + assert!( + response_sender + .send( + EncryptedPassword::try_from("secret") + .expect("test password should be encryptable") + ) + .is_ok() + ); + assert!(password_request.await.is_ok()); + assert!(cancellation.await.is_err()); + assert!( + delegates + .lock() + .get(&1) + .expect("operation should remain registered") + .active_request_cancellation + .is_none() + ); + } + + drop(operation); + } + + #[gpui::test] + async fn concurrent_remote_askpass_request_is_rejected(cx: &mut TestAppContext) { + let delegates = RemoteAskPassDelegates::default(); + let (delegate, mut prompts) = test_askpass_delegate(cx); + let operation = RemoteAskPassOperation::new(1, delegate, delegates.clone()); + let first_request = cx.executor().spawn({ + let delegates = delegates.clone(); + async move { request_remote_password(&delegates, 1, "First:".to_string()).await } + }); + let (_, response_sender, cancellation) = prompts + .next() + .await + .expect("first password prompt should be received"); + + let Err(error) = request_remote_password(&delegates, 1, "Second:".to_string()).await else { + panic!("concurrent prompt should be rejected"); + }; + + assert!(error.to_string().contains("already active")); + assert!(prompts.next().now_or_never().is_none()); + + drop(operation); + assert!(cancellation.await.is_err()); + assert!(first_request.await.is_err()); + drop(response_sender); + } + + #[gpui::test] + async fn remote_askpass_is_rejected_after_operation_ends(cx: &mut TestAppContext) { + let delegates = RemoteAskPassDelegates::default(); + let (delegate, mut prompts) = test_askpass_delegate(cx); + let operation = RemoteAskPassOperation::new(1, delegate, delegates.clone()); + drop(operation); + + let Err(error) = request_remote_password(&delegates, 1, "Password:".to_string()).await + else { + panic!("prompt should be rejected after operation ends"); + }; + + assert!(error.to_string().contains("no longer exists")); + assert!(prompts.next().now_or_never().is_none()); + } + + #[gpui::test] + async fn late_remote_askpass_response_does_not_restore_operation(cx: &mut TestAppContext) { + let delegates = RemoteAskPassDelegates::default(); + let (delegate, mut prompts) = test_askpass_delegate(cx); + let operation = RemoteAskPassOperation::new(1, delegate, delegates.clone()); + let password_request = cx.executor().spawn({ + let delegates = delegates.clone(); + async move { request_remote_password(&delegates, 1, "Password:".to_string()).await } + }); + let (_, response_sender, cancellation) = prompts + .next() + .await + .expect("password prompt should be received"); + + drop(operation); + assert!(cancellation.await.is_err()); + assert!( + response_sender + .send( + EncryptedPassword::try_from("secret") + .expect("test password should be encryptable") + ) + .is_err() + ); + assert!(password_request.await.is_err()); + assert!(delegates.lock().is_empty()); + } + #[test] fn test_is_submodule_git_dir() { assert!(is_submodule_git_dir(Path::new("/Foo/.git/modules/Bar"))); diff --git a/crates/project_benchmarks/src/main.rs b/crates/project_benchmarks/src/main.rs index 31b8bec2cbe..b2c42754872 100644 --- a/crates/project_benchmarks/src/main.rs +++ b/crates/project_benchmarks/src/main.rs @@ -49,6 +49,7 @@ impl RemoteClientDelegate for BenchmarkRemoteClient { &self, prompt: String, tx: oneshot::Sender, + _cancellation: oneshot::Receiver<()>, _cx: &mut gpui::AsyncApp, ) { eprintln!("SSH asking for password: {}", prompt); diff --git a/crates/remote/src/remote_client.rs b/crates/remote/src/remote_client.rs index f63bfa4448d..21ccf58b890 100644 --- a/crates/remote/src/remote_client.rs +++ b/crates/remote/src/remote_client.rs @@ -137,6 +137,7 @@ pub trait RemoteClientDelegate: Send + Sync { &self, prompt: String, tx: oneshot::Sender, + cancellation: oneshot::Receiver<()>, cx: &mut AsyncApp, ); fn get_download_url( diff --git a/crates/remote/src/transport/mock.rs b/crates/remote/src/transport/mock.rs index 3b860ac4e5d..e39d123a1ce 100644 --- a/crates/remote/src/transport/mock.rs +++ b/crates/remote/src/transport/mock.rs @@ -324,6 +324,7 @@ impl RemoteClientDelegate for MockDelegate { &self, _prompt: String, _sender: futures::channel::oneshot::Sender, + _cancellation: futures::channel::oneshot::Receiver<()>, _cx: &mut AsyncApp, ) { unreachable!("MockDelegate::ask_password should not be called in tests") diff --git a/crates/remote/src/transport/ssh.rs b/crates/remote/src/transport/ssh.rs index 8c4dd8a1637..d4399319c06 100644 --- a/crates/remote/src/transport/ssh.rs +++ b/crates/remote/src/transport/ssh.rs @@ -655,9 +655,11 @@ impl SshRemoteConnection { let socket = SshSocket::new(connection_options, reused_path).await?; (socket, None) } else { - let askpass_delegate = askpass::AskPassDelegate::new(cx, { + let askpass_delegate = askpass::AskPassDelegate::new_with_cancellation(cx, { let delegate = delegate.clone(); - move |prompt, tx, cx| delegate.ask_password(prompt, tx, cx) + move |prompt, tx, cancellation, cx| { + delegate.ask_password(prompt, tx, cancellation, cx) + } }); let mut askpass = @@ -717,9 +719,11 @@ impl SshRemoteConnection { #[cfg(windows)] let (socket, master_process_option) = { - let askpass_delegate = askpass::AskPassDelegate::new(cx, { + let askpass_delegate = askpass::AskPassDelegate::new_with_cancellation(cx, { let delegate = delegate.clone(); - move |prompt, tx, cx| delegate.ask_password(prompt, tx, cx) + move |prompt, tx, cancellation, cx| { + delegate.ask_password(prompt, tx, cancellation, cx) + } }); let mut askpass = diff --git a/crates/remote_connection/Cargo.toml b/crates/remote_connection/Cargo.toml index d3b37f6985b..02cd44671c1 100644 --- a/crates/remote_connection/Cargo.toml +++ b/crates/remote_connection/Cargo.toml @@ -31,4 +31,10 @@ settings.workspace = true theme_settings.workspace = true ui.workspace = true ui_input.workspace = true -workspace.workspace = true \ No newline at end of file +workspace.workspace = true + +[dev-dependencies] +editor.workspace = true +gpui = { workspace = true, features = ["test-support"] } +settings = { workspace = true, features = ["test-support"] } +theme.workspace = true diff --git a/crates/remote_connection/src/remote_connection.rs b/crates/remote_connection/src/remote_connection.rs index 395d241a25e..15645b9b9bb 100644 --- a/crates/remote_connection/src/remote_connection.rs +++ b/crates/remote_connection/src/remote_connection.rs @@ -28,6 +28,7 @@ pub struct RemoteConnectionPrompt { is_devcontainer: bool, status_message: Option, prompt: Option<(Entity, oneshot::Sender)>, + prompt_cancellation_task: Option>, cancellation: Option>, editor: Arc, is_password_prompt: bool, @@ -72,6 +73,7 @@ impl RemoteConnectionPrompt { status_message: None, cancellation: None, prompt: None, + prompt_cancellation_task: None, is_password_prompt: false, is_masked: true, } @@ -85,6 +87,7 @@ impl RemoteConnectionPrompt { &mut self, prompt: String, tx: oneshot::Sender, + cancellation: oneshot::Receiver<()>, window: &mut Window, cx: &mut Context, ) { @@ -95,6 +98,14 @@ impl RemoteConnectionPrompt { let markdown = cx.new(|cx| Markdown::new_text(prompt.into(), cx)); self.prompt = Some((markdown, tx)); + self.prompt_cancellation_task = Some(cx.spawn(async move |this, cx| { + cancellation.await.ok(); + this.update(cx, |this, cx| { + this.prompt.take(); + cx.notify(); + }) + .ok(); + })); self.status_message.take(); window.focus(&self.editor.focus_handle(cx), cx); cx.notify(); @@ -107,6 +118,7 @@ impl RemoteConnectionPrompt { pub fn confirm(&mut self, window: &mut Window, cx: &mut Context) { if let Some((_, tx)) = self.prompt.take() { + self.prompt_cancellation_task.take(); self.status_message = Some("Connecting".into()); let pw = self.editor.text(cx); @@ -241,7 +253,7 @@ impl RemoteConnectionModal { (options.distro_name.clone(), None, true, false) } RemoteConnectionOptions::Docker(options) => (options.name.clone(), None, false, true), - #[cfg(any(test, feature = "test-support"))] + #[cfg(feature = "test-support")] RemoteConnectionOptions::Mock(options) => { (format!("mock-{}", options.id), None, false, false) } @@ -452,6 +464,7 @@ impl remote::RemoteClientDelegate for RemoteClientDelegate { &self, prompt: String, tx: oneshot::Sender, + cancellation: oneshot::Receiver<()>, cx: &mut AsyncApp, ) { let mut known_password = self.known_password.clone(); @@ -461,7 +474,7 @@ impl remote::RemoteClientDelegate for RemoteClientDelegate { self.window .update(cx, |_, window, cx| { self.ui.update(cx, |modal, cx| { - modal.set_prompt(prompt, tx, window, cx); + modal.set_prompt(prompt, tx, cancellation, window, cx); }) }) .ok(); @@ -628,6 +641,7 @@ impl remote::RemoteClientDelegate for BackgroundRemoteClientDelegate { &self, prompt: String, _tx: oneshot::Sender, + _cancellation: oneshot::Receiver<()>, _cx: &mut AsyncApp, ) { log::warn!( @@ -726,3 +740,114 @@ pub fn connect( } use anyhow::Context as _; + +#[cfg(test)] +mod tests { + use editor::Editor; + use gpui::TestAppContext; + use settings::SettingsStore; + + use super::*; + + #[gpui::test] + fn clears_prompt_when_password_request_is_cancelled(cx: &mut TestAppContext) { + initialize_test(cx); + + let window = cx.add_window(|window, cx| Editor::single_line(window, cx)); + let prompt = window + .update(cx, |_, window, cx| { + cx.new(|cx| { + RemoteConnectionPrompt::new( + "example.com".to_string(), + None, + false, + false, + window, + cx, + ) + }) + }) + .expect("test window should remain open"); + + let (response_sender, _response_receiver) = oneshot::channel(); + let (cancellation_sender, cancellation) = oneshot::channel(); + window + .update(cx, |_, window, cx| { + prompt.update(cx, |prompt, cx| { + prompt.set_prompt( + "Confirm user presence".to_string(), + response_sender, + cancellation, + window, + cx, + ); + }); + }) + .expect("test window should remain open"); + + drop(cancellation_sender); + cx.run_until_parked(); + + assert!(prompt.read_with(cx, |prompt, _| prompt.prompt.is_none())); + } + + #[gpui::test] + fn stale_cancellation_does_not_clear_replacement_prompt(cx: &mut TestAppContext) { + initialize_test(cx); + + let window = cx.add_window(|window, cx| Editor::single_line(window, cx)); + let prompt = window + .update(cx, |_, window, cx| { + cx.new(|cx| { + RemoteConnectionPrompt::new( + "example.com".to_string(), + None, + false, + false, + window, + cx, + ) + }) + }) + .expect("test window should remain open"); + + let (first_response_sender, _first_response_receiver) = oneshot::channel(); + let (first_cancellation_sender, first_cancellation) = oneshot::channel(); + let (second_response_sender, _second_response_receiver) = oneshot::channel(); + let (_second_cancellation_sender, second_cancellation) = oneshot::channel(); + window + .update(cx, |_, window, cx| { + prompt.update(cx, |prompt, cx| { + prompt.set_prompt( + "First prompt".to_string(), + first_response_sender, + first_cancellation, + window, + cx, + ); + prompt.set_prompt( + "Second prompt".to_string(), + second_response_sender, + second_cancellation, + window, + cx, + ); + }); + }) + .expect("test window should remain open"); + + drop(first_cancellation_sender); + cx.run_until_parked(); + + assert!(prompt.read_with(cx, |prompt, _| prompt.prompt.is_some())); + } + + fn initialize_test(cx: &mut TestAppContext) { + cx.update(|cx| { + let settings_store = SettingsStore::test(cx); + cx.set_global(settings_store); + theme_settings::init(theme::LoadThemes::JustBase, cx); + editor::init(cx); + }); + } +}