From ee9ed936e4f7a4ce58fc7b196f5345b9721b752c Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Wed, 23 Mar 2022 12:15:36 -0600 Subject: [PATCH 1/3] Don't try keychain after authentication fails Previously, we were achieving this by deleting the keychain item, but this can sometimes fail which leads to an infinite loop. Now, we explicitly never try the keychain when reattempting authentication after authentication fails. Co-Authored-By: Max Brunsfeld --- crates/chat_panel/src/chat_panel.rs | 7 +++++- crates/client/src/client.rs | 34 ++++++++++++++--------------- crates/client/src/test.rs | 2 +- crates/project/src/project.rs | 2 +- crates/server/src/rpc.rs | 2 +- crates/zed/src/main.rs | 2 +- 6 files changed, 27 insertions(+), 22 deletions(-) diff --git a/crates/chat_panel/src/chat_panel.rs b/crates/chat_panel/src/chat_panel.rs index bed338d3f43c3ab2e3bbd311b237eed8a015ba34..452f041c7b040c58879caccc19859c379a152042 100644 --- a/crates/chat_panel/src/chat_panel.rs +++ b/crates/chat_panel/src/chat_panel.rs @@ -327,7 +327,12 @@ impl ChatPanel { let rpc = rpc.clone(); let this = this.clone(); cx.spawn(|mut cx| async move { - if rpc.authenticate_and_connect(&cx).log_err().await.is_some() { + if rpc + .authenticate_and_connect(true, &cx) + .log_err() + .await + .is_some() + { cx.update(|cx| { if let Some(this) = this.upgrade(cx) { if this.is_focused(cx) { diff --git a/crates/client/src/client.rs b/crates/client/src/client.rs index d4a5a47b4b509fab8af4eabc9e632a2d952c58e1..db4478f489c07a329871491b13f4a580d8006ef7 100644 --- a/crates/client/src/client.rs +++ b/crates/client/src/client.rs @@ -55,7 +55,7 @@ action!(Authenticate); pub fn init(rpc: Arc, cx: &mut MutableAppContext) { cx.add_global_action(move |_: &Authenticate, cx| { let rpc = rpc.clone(); - cx.spawn(|cx| async move { rpc.authenticate_and_connect(&cx).log_err().await }) + cx.spawn(|cx| async move { rpc.authenticate_and_connect(true, &cx).log_err().await }) .detach(); }); } @@ -302,7 +302,7 @@ impl Client { state._reconnect_task = Some(cx.spawn(|cx| async move { let mut rng = StdRng::from_entropy(); let mut delay = Duration::from_millis(100); - while let Err(error) = this.authenticate_and_connect(&cx).await { + while let Err(error) = this.authenticate_and_connect(true, &cx).await { log::error!("failed to connect {}", error); this.set_status( Status::ReconnectionError { @@ -547,6 +547,7 @@ impl Client { #[async_recursion(?Send)] pub async fn authenticate_and_connect( self: &Arc, + try_keychain: bool, cx: &AsyncAppContext, ) -> anyhow::Result<()> { let was_disconnected = match *self.status().borrow() { @@ -568,23 +569,22 @@ impl Client { self.set_status(Status::Reauthenticating, cx) } - let mut used_keychain = false; - let credentials = self.state.read().credentials.clone(); - let credentials = if let Some(credentials) = credentials { - credentials - } else if let Some(credentials) = read_credentials_from_keychain(cx) { - used_keychain = true; - credentials - } else { - let credentials = match self.authenticate(&cx).await { + let mut read_from_keychain = false; + let mut credentials = self.state.read().credentials.clone(); + if credentials.is_none() && try_keychain { + credentials = read_credentials_from_keychain(cx); + read_from_keychain = credentials.is_some(); + } + if credentials.is_none() { + credentials = Some(match self.authenticate(&cx).await { Ok(credentials) => credentials, Err(err) => { self.set_status(Status::ConnectionError, cx); return Err(err); } - }; - credentials - }; + }); + } + let credentials = credentials.unwrap(); if was_disconnected { self.set_status(Status::Connecting, cx); @@ -595,7 +595,7 @@ impl Client { match self.establish_connection(&credentials, cx).await { Ok(conn) => { self.state.write().credentials = Some(credentials.clone()); - if !used_keychain && IMPERSONATE_LOGIN.is_none() { + if !read_from_keychain && IMPERSONATE_LOGIN.is_none() { write_credentials_to_keychain(&credentials, cx).log_err(); } self.set_connection(conn, cx).await; @@ -603,10 +603,10 @@ impl Client { } Err(EstablishConnectionError::Unauthorized) => { self.state.write().credentials.take(); - if used_keychain { + if read_from_keychain { cx.platform().delete_credentials(&ZED_SERVER_URL).log_err(); self.set_status(Status::SignedOut, cx); - self.authenticate_and_connect(cx).await + self.authenticate_and_connect(false, cx).await } else { self.set_status(Status::ConnectionError, cx); Err(EstablishConnectionError::Unauthorized)? diff --git a/crates/client/src/test.rs b/crates/client/src/test.rs index f630d9c0ee0a08fc76753b39817d3665afeae4fc..35a8e85922b153ce3d997ac8901dd70992adfe9d 100644 --- a/crates/client/src/test.rs +++ b/crates/client/src/test.rs @@ -91,7 +91,7 @@ impl FakeServer { }); client - .authenticate_and_connect(&cx.to_async()) + .authenticate_and_connect(false, &cx.to_async()) .await .unwrap(); server diff --git a/crates/project/src/project.rs b/crates/project/src/project.rs index c3c2f4e2a036e52c58b76a8a85d985f33180aa94..fbd3a1f7a6026a0d60361e33e4d801f0d79a6897 100644 --- a/crates/project/src/project.rs +++ b/crates/project/src/project.rs @@ -377,7 +377,7 @@ impl Project { fs: Arc, cx: &mut AsyncAppContext, ) -> Result> { - client.authenticate_and_connect(&cx).await?; + client.authenticate_and_connect(true, &cx).await?; let response = client .request(proto::JoinProject { diff --git a/crates/server/src/rpc.rs b/crates/server/src/rpc.rs index 1020fd686a2c38fb7ca0f77ffabd81d21d6c5ada..68e435dfb1c2a3aa45b8a95131c52146ff4136a9 100644 --- a/crates/server/src/rpc.rs +++ b/crates/server/src/rpc.rs @@ -5021,7 +5021,7 @@ mod tests { }); client - .authenticate_and_connect(&cx.to_async()) + .authenticate_and_connect(false, &cx.to_async()) .await .unwrap(); diff --git a/crates/zed/src/main.rs b/crates/zed/src/main.rs index 61967bfcdfb00738c56d52fa228041a30f9e90ce..29dfd314273e20f9142e66dfaf847a634e4a8271 100644 --- a/crates/zed/src/main.rs +++ b/crates/zed/src/main.rs @@ -82,7 +82,7 @@ fn main() { let client = client.clone(); |cx| async move { if client.has_keychain_credentials(&cx) { - client.authenticate_and_connect(&cx).await?; + client.authenticate_and_connect(true, &cx).await?; } Ok::<_, anyhow::Error>(()) } From 657b92b02078688fcdaff715067ddfc260238b01 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Wed, 23 Mar 2022 12:18:17 -0600 Subject: [PATCH 2/3] Don't prompt for keychain access when launching from a pty Co-Authored-By: Max Brunsfeld --- crates/zed/src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/zed/src/main.rs b/crates/zed/src/main.rs index 29dfd314273e20f9142e66dfaf847a634e4a8271..af0c98506d38480003796c6321c5dea7c5765d1d 100644 --- a/crates/zed/src/main.rs +++ b/crates/zed/src/main.rs @@ -81,7 +81,7 @@ fn main() { cx.spawn({ let client = client.clone(); |cx| async move { - if client.has_keychain_credentials(&cx) { + if !stdout_is_a_pty() && client.has_keychain_credentials(&cx) { client.authenticate_and_connect(true, &cx).await?; } Ok::<_, anyhow::Error>(()) From 4a42025c286a987b5f962419ea14576bbdede356 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Wed, 23 Mar 2022 12:25:06 -0600 Subject: [PATCH 3/3] Authenticate on startup if ZED_IMPERSONATE is assigned Co-Authored-By: Max Brunsfeld --- crates/client/src/client.rs | 2 +- crates/zed/src/main.rs | 10 ++++++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/crates/client/src/client.rs b/crates/client/src/client.rs index db4478f489c07a329871491b13f4a580d8006ef7..1bae6cd49e5eb9f5e24b86f6165b7a647c6a30e8 100644 --- a/crates/client/src/client.rs +++ b/crates/client/src/client.rs @@ -45,7 +45,7 @@ pub use user::*; lazy_static! { static ref ZED_SERVER_URL: String = std::env::var("ZED_SERVER_URL").unwrap_or("https://zed.dev".to_string()); - static ref IMPERSONATE_LOGIN: Option = std::env::var("ZED_IMPERSONATE") + pub static ref IMPERSONATE_LOGIN: Option = std::env::var("ZED_IMPERSONATE") .ok() .and_then(|s| if s.is_empty() { None } else { Some(s) }); } diff --git a/crates/zed/src/main.rs b/crates/zed/src/main.rs index af0c98506d38480003796c6321c5dea7c5765d1d..b79f7f1bb0966c5df12a1612b8c33015576e3ddd 100644 --- a/crates/zed/src/main.rs +++ b/crates/zed/src/main.rs @@ -81,8 +81,14 @@ fn main() { cx.spawn({ let client = client.clone(); |cx| async move { - if !stdout_is_a_pty() && client.has_keychain_credentials(&cx) { - client.authenticate_and_connect(true, &cx).await?; + if stdout_is_a_pty() { + if client::IMPERSONATE_LOGIN.is_some() { + client.authenticate_and_connect(false, &cx).await?; + } + } else { + if client.has_keychain_credentials(&cx) { + client.authenticate_and_connect(true, &cx).await?; + } } Ok::<_, anyhow::Error>(()) }