Allow signing in again if authentication is pending or was unsuccessful

Antonio Scandurra created 3 years ago

The local server that we spin up to receive OAuth callbacks isn't
called when an error occurs and it is non-trivial to do so with
next-auth. Besides, there could be cases where the user explicitly
closes the browser window before the callback can be invoked.

With this commit, the user can sign in even while an authentication
is still in progress. As opposed to waiting for at most 10 minutes
before killing the local HTTP server if we haven't received the callback,
we will repeatedly check for a response every second for 100 seconds.
This gives us a chance to determine whether a new authentication has started
in the meantime and, if so, abort the current authentication flow.

Change summary

crates/client/src/client.rs       | 99 ++++++++++++++++++--------------
crates/workspace/src/workspace.rs |  2 
2 files changed, 56 insertions(+), 45 deletions(-)

Detailed changes

crates/client/src/client.rs

@@ -569,14 +569,14 @@ impl Client {
     ) -> anyhow::Result<()> {
         let was_disconnected = match *self.status().borrow() {
             Status::SignedOut => true,
-            Status::ConnectionError | Status::ConnectionLost | Status::ReconnectionError { .. } => {
-                false
+            Status::ConnectionError
+            | Status::ConnectionLost
+            | Status::Authenticating { .. }
+            | Status::Reauthenticating { .. }
+            | Status::ReconnectionError { .. } => false,
+            Status::Connected { .. } | Status::Connecting { .. } | Status::Reconnecting { .. } => {
+                return Ok(())
             }
-            Status::Connected { .. }
-            | Status::Connecting { .. }
-            | Status::Reconnecting { .. }
-            | Status::Authenticating
-            | Status::Reauthenticating => return Ok(()),
             Status::UpgradeRequired => return Err(EstablishConnectionError::UpgradeRequired)?,
         };
 
@@ -593,13 +593,22 @@ impl Client {
             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);
+            let mut status_rx = self.status();
+            let _ = status_rx.next().await;
+            futures::select_biased! {
+                authenticate = self.authenticate(&cx).fuse() => {
+                    match authenticate {
+                        Ok(creds) => credentials = Some(creds),
+                        Err(err) => {
+                            self.set_status(Status::ConnectionError, cx);
+                            return Err(err);
+                        }
+                    }
                 }
-            });
+                _ = status_rx.next().fuse() => {
+                    return Err(anyhow!("authentication canceled"));
+                }
+            }
         }
         let credentials = credentials.unwrap();
 
@@ -899,40 +908,42 @@ impl Client {
             // custom URL scheme instead of this local HTTP server.
             let (user_id, access_token) = executor
                 .spawn(async move {
-                    if let Some(req) = server.recv_timeout(Duration::from_secs(10 * 60))? {
-                        let path = req.url();
-                        let mut user_id = None;
-                        let mut access_token = None;
-                        let url = Url::parse(&format!("http://example.com{}", path))
-                            .context("failed to parse login notification url")?;
-                        for (key, value) in url.query_pairs() {
-                            if key == "access_token" {
-                                access_token = Some(value.to_string());
-                            } else if key == "user_id" {
-                                user_id = Some(value.to_string());
+                    for _ in 0..100 {
+                        if let Some(req) = server.recv_timeout(Duration::from_secs(1))? {
+                            let path = req.url();
+                            let mut user_id = None;
+                            let mut access_token = None;
+                            let url = Url::parse(&format!("http://example.com{}", path))
+                                .context("failed to parse login notification url")?;
+                            for (key, value) in url.query_pairs() {
+                                if key == "access_token" {
+                                    access_token = Some(value.to_string());
+                                } else if key == "user_id" {
+                                    user_id = Some(value.to_string());
+                                }
                             }
-                        }
 
-                        let post_auth_url =
-                            format!("{}/native_app_signin_succeeded", *ZED_SERVER_URL);
-                        req.respond(
-                            tiny_http::Response::empty(302).with_header(
-                                tiny_http::Header::from_bytes(
-                                    &b"Location"[..],
-                                    post_auth_url.as_bytes(),
-                                )
-                                .unwrap(),
-                            ),
-                        )
-                        .context("failed to respond to login http request")?;
-                        Ok((
-                            user_id.ok_or_else(|| anyhow!("missing user_id parameter"))?,
-                            access_token
-                                .ok_or_else(|| anyhow!("missing access_token parameter"))?,
-                        ))
-                    } else {
-                        Err(anyhow!("didn't receive login redirect"))
+                            let post_auth_url =
+                                format!("{}/native_app_signin_succeeded", *ZED_SERVER_URL);
+                            req.respond(
+                                tiny_http::Response::empty(302).with_header(
+                                    tiny_http::Header::from_bytes(
+                                        &b"Location"[..],
+                                        post_auth_url.as_bytes(),
+                                    )
+                                    .unwrap(),
+                                ),
+                            )
+                            .context("failed to respond to login http request")?;
+                            return Ok((
+                                user_id.ok_or_else(|| anyhow!("missing user_id parameter"))?,
+                                access_token
+                                    .ok_or_else(|| anyhow!("missing access_token parameter"))?,
+                            ));
+                        }
                     }
+
+                    Err(anyhow!("didn't receive login redirect"))
                 })
                 .await?;
 

crates/workspace/src/workspace.rs

@@ -1811,7 +1811,7 @@ impl Workspace {
         match &*self.client.status().borrow() {
             client::Status::ConnectionError
             | client::Status::ConnectionLost
-            | client::Status::Reauthenticating
+            | client::Status::Reauthenticating { .. }
             | client::Status::Reconnecting { .. }
             | client::Status::ReconnectionError { .. } => Some(
                 Container::new(