[geary/wip/17-noisy-problem-reports: 9/12] Rework how auth failures are propagated by the IMAP stack

[Michael Gratton <mjog src gnome org>]

commit 20a53f2d51b28c44a8f38192b20f76da3b35d417
Author: Michael Gratton <mike vee net>
Date:   Tue Jan 1 21:59:11 2019 +1100

    Rework how auth failures are propagated by the IMAP stack
    
    Rather than using cascading signals to indicate an auth error, have
    ImapClientSession throw an appropriate exception, and catch that as
    needed in ImapClientService and the Engine's validation code.

 src/engine/api/geary-engine.vala                   | 29 +++---------
 src/engine/imap/api/imap-client-service.vala       | 38 ++++++----------
 src/engine/imap/transport/imap-client-session.vala | 51 +++++++++++++++-------
 3 files changed, 55 insertions(+), 63 deletions(-)
---
diff --git a/src/engine/api/geary-engine.vala b/src/engine/api/geary-engine.vala
index 66bedf3a..976431d0 100644
--- a/src/engine/api/geary-engine.vala
+++ b/src/engine/api/geary-engine.vala
@@ -243,28 +243,20 @@ public class Geary.Engine : BaseObject {
         );
 
         Geary.Imap.ClientSession client = new Imap.ClientSession(endpoint);
-        GLib.Error? connect_err = null;
+        GLib.Error? imap_err = null;
         try {
             yield client.connect_async(cancellable);
         } catch (GLib.Error err) {
-            connect_err = err;
+            imap_err = err;
         }
 
-        bool login_failed = false;
-        GLib.Error? login_err = null;
-        if (connect_err == null) {
-            // XXX initiate_session_async doesn't seem to actually
-            // throw an imap error on login failed. This is not worth
-            // fixing until wip/26-proton-mail-bridge lands though, so
-            // use signals as a workaround instead.
-            client.login_failed.connect(() => login_failed = true);
-
+        if (imap_err == null) {
             try {
                 yield client.initiate_session_async(
                     service.credentials, cancellable
                 );
             } catch (GLib.Error err) {
-                login_err = err;
+                imap_err = err;
             }
 
             try {
@@ -278,17 +270,8 @@ public class Geary.Engine : BaseObject {
         // error
         endpoint.disconnect(untrusted_id);
 
-        if (connect_err != null) {
-            throw connect_err;
-        }
-
-        if (login_failed) {
-            // XXX This should be a LOGIN_FAILED error or something
-            throw new ImapError.UNAUTHENTICATED("Login failed");
-        }
-
-        if (login_err != null) {
-            throw login_err;
+        if (imap_err != null) {
+            throw imap_err;
         }
     }
 
diff --git a/src/engine/imap/api/imap-client-service.vala b/src/engine/imap/api/imap-client-service.vala
index 2361d710..5b2c315d 100644
--- a/src/engine/imap/api/imap-client-service.vala
+++ b/src/engine/imap/api/imap-client-service.vala
@@ -282,13 +282,22 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
             ClientSession free = yield this.create_new_authorized_session(
                 this.pool_cancellable
             );
+            notify_connected();
             yield this.sessions_mutex.execute_locked(() => {
                     this.all_sessions.add(free);
                 });
             this.free_queue.send(free);
-        } catch (Error err) {
-            debug("[%s] Error adding new session to the pool: %s",
+        } catch (ImapError.UNAUTHENTICATED err) {
+            debug("[%s] Auth error adding new session to the pool: %s",
                   this.account.id, err.message);
+            notify_authentication_failed();
+            this.close_pool.begin();
+        } catch (Error err) {
+            if (!(err is IOError.CANCELLED)) {
+                debug("[%s] Error adding new session to the pool: %s",
+                      this.account.id, err.message);
+                notify_connection_failed(err);
+            }
             this.close_pool.begin();
         }
     }
@@ -357,12 +366,6 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
     private async ClientSession create_new_authorized_session(Cancellable? cancellable) throws Error {
         debug("[%s] Opening new session", this.account.id);
         ClientSession new_session = new ClientSession(remote);
-
-        // Listen for auth failures early so the client is notified if
-        // there is an error, even though we won't want to keep the
-        // session around.
-        new_session.login_failed.connect(on_login_failed);
-
         try {
             yield new_session.connect_async(cancellable);
         } catch (GLib.Error err) {
@@ -376,16 +379,10 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
             yield new_session.initiate_session_async(
                 this.configuration.credentials, cancellable
             );
-        } catch (ImapError.UNAUTHENTICATED err) {
-            notify_authentication_failed();
-            throw err;
         } catch (Error err) {
-            if (!(err is IOError.CANCELLED)) {
-                notify_connection_failed(err);
-            }
-
-            // need to disconnect before throwing error ... don't honor Cancellable here, it's
-            // important to disconnect the client before dropping the ref
+            // need to disconnect before throwing error ... don't
+            // honor Cancellable here, it's important to disconnect
+            // the client before dropping the ref
             try {
                 yield new_session.disconnect_async();
             } catch (Error disconnect_err) {
@@ -403,7 +400,6 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
                                       unselected_keepalive_sec,
                                       selected_with_idle_keepalive_sec);
 
-        notify_connected();
         return new_session;
     }
 
@@ -458,7 +454,6 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
 
         if (removed) {
             session.disconnected.disconnect(on_disconnected);
-            session.login_failed.disconnect(on_login_failed);
         }
         return removed;
     }
@@ -477,9 +472,4 @@ internal class Geary.Imap.ClientService : Geary.ClientService {
         );
     }
 
-    private void on_login_failed(ClientSession session, StatusResponse? response) {
-        notify_authentication_failed();
-        this.close_pool.begin();
-    }
-
 }
diff --git a/src/engine/imap/transport/imap-client-session.vala 
b/src/engine/imap/transport/imap-client-session.vala
index 60e87338..ef3d44c7 100644
--- a/src/engine/imap/transport/imap-client-session.vala
+++ b/src/engine/imap/transport/imap-client-session.vala
@@ -827,17 +827,14 @@ public class Geary.Imap.ClientSession : BaseObject {
         
         return State.LOGGED_OUT;
     }
-    
-    //
-    // login
-    //
 
     /**
      * Performs the LOGIN command using the supplied credentials.
      *
      * @see initiate_session_async
      */
-    public async StatusResponse login_async(Geary.Credentials credentials, Cancellable? cancellable = null)
+    public async StatusResponse login_async(Geary.Credentials credentials,
+                                            Cancellable? cancellable = null)
         throws Error {
         Command? cmd = null;
         switch (credentials.supported_method) {
@@ -869,16 +866,42 @@ public class Geary.Imap.ClientSession : BaseObject {
 
         MachineParams params = new MachineParams(cmd);
         fsm.issue(Event.LOGIN, null, params);
-        
+
         if (params.err != null)
             throw params.err;
-        
+
         // should always proceed; only an Error could change this
         assert(params.proceed);
-        
-        return yield command_transaction_async(cmd, cancellable);
+
+        GLib.Error? login_err = null;
+        try {
+            yield command_transaction_async(cmd, cancellable);
+        } catch (Error err) {
+            login_err = err;
+        }
+
+        if (login_err != null) {
+            // Throw an error indicating auth failed here, unless the
+            // response code indicated that the server is merely
+            // unavailable, then don't since the creds might actually
+            // be fine.
+            ResponseCode? code = cmd.status.response_code;
+            ResponseCodeType? code_type = null;
+            if (code != null) {
+                code_type = code.get_response_code_type();
+            }
+
+            if (code_type == null ||
+                code_type.value != ResponseCodeType.UNAVAILABLE) {
+                throw new ImapError.UNAUTHENTICATED(login_err.message);
+            } else {
+                throw login_err;
+            }
+        }
+
+        return cmd.status;
     }
-    
+
     /**
      * Prepares the connection and performs a login using the supplied credentials.
      *
@@ -931,12 +954,8 @@ public class Geary.Imap.ClientSession : BaseObject {
         }
 
         // Login after STARTTLS
-        StatusResponse login_resp = yield login_async(credentials, cancellable);
-        if (login_resp.status != Status.OK) {
-            throw new ImapError.UNAUTHENTICATED("Unable to login to %s with supplied credentials",
-                to_string());
-        }
-        
+        yield login_async(credentials, cancellable);
+
         // if new capabilities not offered after login, get them now
         if (caps.revision == capabilities.revision)
             yield send_command_async(new CapabilityCommand());