[geary/wip/713006-better-error-reporting: 3/6] Push IMAP auth handling down the stack, try 3 times before bailing out.

[Michael Gratton <mjog src gnome org>]

commit 7310e09c78ca1b319549e76864f3841c31e96ed1
Author: Michael James Gratton <mike vee net>
Date:   Thu Nov 9 17:43:30 2017 +1100

    Push IMAP auth handling down the stack, try 3 times before bailing out.
    
    * src/engine/api/geary-account.vala (Account): Add internal constant for
      maximum number of authentication attempts.
    
    * src/engine/imap-engine/imap-engine-generic-account.vala (Account):
      Move auth-related code to Imap.Account.
    
    * src/engine/imap/api/imap-account.vala (Account): Keep track of the
      number of auth failures and bail out when the max has been
      reached. Manually notify the ClientSessionManager when the user has
      updated their creds so it only restarts as a response to user input.
    
    * src/engine/imap/transport/imap-client-session-manager.vala
      (ClientSessionManager): Don't keep track of imap credentials changing,
      add new credentials_updated() method to allow Imap.Account
      notify when they have changed instead.

 src/engine/api/geary-account.vala                  |    5 +
 .../imap-engine/imap-engine-generic-account.vala   |   40 +---------
 src/engine/imap/api/imap-account.vala              |   83 ++++++++++++++++---
 .../transport/imap-client-session-manager.vala     |   26 ++++---
 4 files changed, 92 insertions(+), 62 deletions(-)
---
diff --git a/src/engine/api/geary-account.vala b/src/engine/api/geary-account.vala
index 148127f..bd3145e 100644
--- a/src/engine/api/geary-account.vala
+++ b/src/engine/api/geary-account.vala
@@ -21,6 +21,11 @@
  */
 
 public abstract class Geary.Account : BaseObject {
+
+
+    /** Number of times to attempt re-authentication. */
+    internal const uint AUTH_ATTEMPTS_MAX = 3;
+
     public enum Problem {
         CONNECTION_FAILURE,
         DATABASE_FAILURE,
diff --git a/src/engine/imap-engine/imap-engine-generic-account.vala 
b/src/engine/imap-engine/imap-engine-generic-account.vala
index f9b5751..be64f58 100644
--- a/src/engine/imap-engine/imap-engine-generic-account.vala
+++ b/src/engine/imap-engine/imap-engine-generic-account.vala
@@ -29,7 +29,6 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account {
         = new Gee.HashMap<FolderPath, uint>();
     private Gee.HashSet<Geary.Folder> in_refresh_unseen = new Gee.HashSet<Geary.Folder>();
     private AccountSynchronizer sync;
-    private bool awaiting_credentials = false;
     private Cancellable? enumerate_folder_cancellable = null;
     private TimeoutManager refresh_folder_timer;
 
@@ -43,10 +42,9 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account {
 
         this.remote = remote;
         this.remote.ready.connect(on_remote_ready);
+        this.remote.report_problem.connect(notify_report_problem);
 
         this.local = local;
-        
-        this.remote.login_failed.connect(on_login_failed);
         this.local.contacts_loaded.connect(() => { contacts_loaded(); });
         this.local.email_sent.connect(on_email_sent);
 
@@ -959,44 +957,8 @@ private abstract class Geary.ImapEngine.GenericAccount : Geary.Account {
         return yield local.get_containing_folders_async(ids, cancellable);
     }
 
-    private void on_login_failed(Geary.Credentials? credentials, Geary.Imap.StatusResponse? response) {
-        if (awaiting_credentials)
-            return; // We're already asking for the password.
-        
-        awaiting_credentials = true;
-        do_login_failed_async.begin(credentials, response, () => { awaiting_credentials = false; });
-    }
-
     private void on_remote_ready() {
         this.enumerate_folders_async.begin();
     }
 
-    private async void do_login_failed_async(Geary.Credentials? credentials, Geary.Imap.StatusResponse? 
response) {
-        bool reask_password = true;
-        try {
-            reask_password = (
-                response == null ||
-                response.response_code == null ||
-                response.response_code.get_response_code_type().value != 
Geary.Imap.ResponseCodeType.UNAVAILABLE
-            );
-        } catch (ImapError ierr) {
-            debug("Unable to parse ResponseCode %s: %s", response.response_code.to_string(),
-                ierr.message);
-        }
-        // login can fail due to an invalid password hence we should re-ask it
-        // but it can also fail due to server inaccessibility, for instance "[UNAVAILABLE] /
-        // Maximum number of connections from user+IP exceeded". In that case, resetting password seems 
unneeded.
-        if (reask_password) {
-            try {
-                if (yield information.fetch_passwords_async(ServiceFlag.IMAP, true))
-                    return;
-            } catch (Error e) {
-                debug("Error prompting for IMAP password: %s", e.message);
-            }
-            notify_report_problem(Geary.Account.Problem.RECV_EMAIL_LOGIN_FAILED, null);
-        } else {
-            notify_report_problem(Geary.Account.Problem.CONNECTION_FAILURE, null);
-        }
-    }
 }
-
diff --git a/src/engine/imap/api/imap-account.vala b/src/engine/imap/api/imap-account.vala
index fec5610..050992d 100644
--- a/src/engine/imap/api/imap-account.vala
+++ b/src/engine/imap/api/imap-account.vala
@@ -30,8 +30,9 @@ private class Geary.Imap.Account : BaseObject {
     public bool is_ready { get { return this.session_mgr.is_ready; } }
 
     private string name;
-    private AccountInformation account_information;
+    private AccountInformation account;
     private ClientSessionManager session_mgr;
+    private uint authentication_failures = 0;
     private ClientSession? account_session = null;
     private Nonblocking.Mutex account_session_mutex = new Nonblocking.Mutex();
     private Nonblocking.Mutex cmd_mutex = new Nonblocking.Mutex();
@@ -52,16 +53,17 @@ private class Geary.Imap.Account : BaseObject {
      */
     public signal void ready();
 
-    public signal void login_failed(Geary.Credentials? cred, StatusResponse? response);
+    /** Fired if a user-notifiable problem occurs. */
+    public signal void report_problem(Geary.Account.Problem problem, Error? err);
 
-    public Account(Geary.AccountInformation account_information) {
-        this.account_information = account_information;
-        this.session_mgr = new ClientSessionManager(account_information);
+    public Account(Geary.AccountInformation account) {
         this.name = account.id + ":imap";
+        this.account = account;
+        this.session_mgr = new ClientSessionManager(account);
         this.session_mgr.ready.connect(on_session_ready);
         this.session_mgr.login_failed.connect(on_login_failed);
     }
-    
+
     private void check_open() throws Error {
         if (!is_open)
             throw new EngineError.OPEN_REQUIRED("Imap.Account not open");
@@ -70,9 +72,15 @@ private class Geary.Imap.Account : BaseObject {
     public async void open_async(Cancellable? cancellable = null) throws Error {
         if (is_open)
             throw new EngineError.ALREADY_OPEN("Imap.Account already open");
-        
+
+        // Reset this so we start trying to authenticate again
+        this.authentication_failures = 0;
+
+        // This will cause the session manager to open at least one
+        // connection. We can't attempt to claim one straight away
+        // since we might not be online.
         yield session_mgr.open_async(cancellable);
-        
+
         is_open = true;
     }
     
@@ -618,17 +626,66 @@ private class Geary.Imap.Account : BaseObject {
         throw new EngineError.NOT_FOUND("Folder %s not found on %s",
             (path != null) ? path.to_string() : "root", session_mgr.to_string());
     }
-    
-    private void on_login_failed(StatusResponse? response) {
-        login_failed(account_information.imap_credentials, response);
-    }
 
     private void on_session_ready() {
+        // Now have a valid session, so credentials must be good
+        this.authentication_failures = 0;
         ready();
     }
 
+    private void on_login_failed(Geary.Imap.StatusResponse? response) {
+        this.authentication_failures++;
+        if (this.authentication_failures >= Geary.Account.AUTH_ATTEMPTS_MAX) {
+            // We have tried auth too many times, so bail out
+            report_problem(Geary.Account.Problem.RECV_EMAIL_LOGIN_FAILED, null);
+        } else {
+            // login can fail due to an invalid password hence we
+            // should re-ask it but it can also fail due to server
+            // inaccessibility, for instance "[UNAVAILABLE] / Maximum
+            // number of connections from user+IP exceeded". In that
+            // case, resetting password seems unneeded.
+            bool reask_password = false;
+            Error? login_error = null;
+            try {
+                reask_password = (
+                    response == null ||
+                    response.response_code == null ||
+                    response.response_code.get_response_code_type().value != 
Geary.Imap.ResponseCodeType.UNAVAILABLE
+                );
+            } catch (ImapError err) {
+                login_error = err;
+                debug("Unable to parse ResponseCode %s: %s", response.response_code.to_string(),
+                      err.message);
+            }
+
+            if (!reask_password) {
+                // Either the server was unavailable, or we were unable to
+                // parse the login response. Either way, indicate a
+                // non-login error.
+                report_problem(Geary.Account.Problem.CONNECTION_FAILURE, login_error);
+            } else {
+                // Now, we should ask the user for their password
+                this.account.fetch_passwords_async.begin(
+                    ServiceFlag.IMAP, true,
+                    (obj, ret) => {
+                        try {
+                            if (this.account.fetch_passwords_async.end(ret)) {
+                                // Have a new password, so try that
+                                this.session_mgr.credentials_updated();
+                            } else {
+                                // User cancelled, so indicate a login problem
+                                report_problem(Geary.Account.Problem.RECV_EMAIL_LOGIN_FAILED, null);
+                            }
+                        } catch (Error err) {
+                            report_problem(Geary.Account.Problem.CONNECTION_FAILURE, err);
+                        }
+                    });
+            }
+        }
+    }
+
     public string to_string() {
         return name;
     }
-}
 
+}
diff --git a/src/engine/imap/transport/imap-client-session-manager.vala 
b/src/engine/imap/transport/imap-client-session-manager.vala
index ef87601..e3fdbe2 100644
--- a/src/engine/imap/transport/imap-client-session-manager.vala
+++ b/src/engine/imap/transport/imap-client-session-manager.vala
@@ -84,9 +84,9 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
     /** Fired when an authentication error occurs opening a session. */
     public signal void login_failed(StatusResponse? response);
 
+
     public ClientSessionManager(AccountInformation account_information) {
         this.account_information = account_information;
-        this.account_information.notify["imap-credentials"].connect(on_imap_credentials_notified);
 
         // NOTE: This works because AccountInformation guarantees the IMAP endpoint not to change
         // for the lifetime of the AccountInformation object; if this ever changes, will need to
@@ -115,7 +115,6 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
         if (is_open)
             warning("Destroying opened ClientSessionManager");
 
-        this.account_information.notify["imap-credentials"].disconnect(on_imap_credentials_notified);
         this.endpoint.untrusted_host.disconnect(on_imap_untrusted_host);
         this.endpoint.notify[Endpoint.PROP_TRUST_UNTRUSTED_HOST].disconnect(on_imap_trust_untrusted_host);
     }
@@ -124,7 +123,8 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
         if (is_open)
             throw new EngineError.ALREADY_OPEN("ClientSessionManager already open");
 
-        is_open = true;
+        this.is_open = true;
+        this.authentication_failed = false;
 
                this.endpoint.connectivity.notify["is-reachable"].connect(on_connectivity_change);
         if (this.endpoint.connectivity.is_reachable) {
@@ -172,14 +172,20 @@ public class Geary.Imap.ClientSessionManager : BaseObject {
                 break;
         }
     }
-    
-    private void on_imap_credentials_notified() {
-        authentication_failed = false;
-        
-        if (is_open)
-            adjust_session_pool.begin();
+
+    /**
+     * Informs the manager that the account's IMAP credentials have changed.
+     *
+     * This will reset the manager's authentication state and if open,
+     * attempt to open a connection to the server.
+     */
+    public void credentials_updated() {
+        this.authentication_failed = false;
+        if (this.is_open) {
+            this.adjust_session_pool.begin();
+        }
     }
-    
+
     private void check_open() throws Error {
         if (!is_open)
             throw new EngineError.OPEN_REQUIRED("ClientSessionManager is not open");