From ee9ed936e4f7a4ce58fc7b196f5345b9721b752c Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Wed, 23 Mar 2022 12:15:36 -0600 Subject: [PATCH] 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 bed338d3f4..452f041c7b 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 d4a5a47b4b..db4478f489 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 f630d9c0ee..35a8e85922 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 c3c2f4e2a0..fbd3a1f7a6 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 1020fd686a..68e435dfb1 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 61967bfcdf..29dfd31427 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>(()) }