[geary/mjog/imap-connection-fixes: 3/4] Geary.Imap.{Acount, Folder, Client}Session: Handle NO responses better

[Michael Gratton <mjog src gnome org>]

commit 43dfe338ba9a3a5388f3632c017c486d64569496
Author: Michael Gratton <mike vee net>
Date:   Fri Mar 27 08:23:13 2020 +1100

    Geary.Imap.{Acount,Folder,Client}Session: Handle NO responses better
    
    Instead of treading `NO` and `BAD` responses by throwing a
    `SERVER_ERROR`, call `Command.throw_on_error` so that NO throws an
    `OPERATIONAL_ERROR` and any IMAP response codes returned are also
    observed.
    
    In ClientSession's high-level protocol methods (`login_async`,
    `select_async`, etc), just check the error as well rather than passing
    the response back and expecting the caller to do it.
    
    Fixes #502

 src/engine/imap/api/imap-account-session.vala      | 52 +++++------------
 src/engine/imap/api/imap-folder-session.vala       | 27 +--------
 src/engine/imap/transport/imap-client-session.vala | 66 +++++++---------------
 3 files changed, 37 insertions(+), 108 deletions(-)
---
diff --git a/src/engine/imap/api/imap-account-session.vala b/src/engine/imap/api/imap-account-session.vala
index f0bdfef1..2131c844 100644
--- a/src/engine/imap/api/imap-account-session.vala
+++ b/src/engine/imap/api/imap-account-session.vala
@@ -101,16 +101,7 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject {
             ? new CreateCommand.special_use(mailbox, type)
             : new CreateCommand(mailbox);
 
-        StatusResponse response = yield send_command_async(
-            session, cmd, null, null, cancellable
-        );
-
-        if (response.status != Status.OK) {
-            throw new ImapError.SERVER_ERROR(
-                "Server reports error creating folder %s: %s",
-                mailbox.to_string(), response.to_string()
-            );
-        }
+        yield send_command_async(session, cmd, null, null, cancellable);
     }
 
     /**
@@ -339,11 +330,7 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject {
         }
 
         Gee.List<MailboxInformation> list_results = new Gee.ArrayList<MailboxInformation>();
-        StatusResponse response = yield send_command_async(session, cmd, list_results, null, cancellable);
-        if (response.status != Status.OK) {
-            throw new ImapError.SERVER_ERROR("Unable to list children of %s: %s",
-                (folder != null) ? folder.to_string() : "root", response.to_string());
-        }
+        yield send_command_async(session, cmd, list_results, null, cancellable);
 
         // See note at ListCommand about some servers returning the
         // parent's name alongside their children ... this filters
@@ -369,36 +356,26 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject {
                                                StatusDataType[] status_types,
                                                Cancellable? cancellable)
     throws Error {
-        Gee.List<StatusData> status_results = new Gee.ArrayList<StatusData>();
-        StatusResponse response = yield send_command_async(
-            session,
-            new StatusCommand(mailbox, status_types),
-            null,
-            status_results,
-            cancellable
+        var status_results = new Gee.ArrayList<StatusData>();
+        var cmd = new StatusCommand(mailbox, status_types);
+        yield send_command_async(
+            session, cmd, null, status_results, cancellable
         );
-
-        if (response.status != Status.OK) {
-            throw new ImapError.SERVER_ERROR("Error fetching \"%s\" STATUS: %s",
-                                             mailbox.to_string(),
-                                             response.to_string());
-        }
-
         if (status_results.size != 1) {
             throw new ImapError.INVALID("Invalid result count (%d) \"%s\" STATUS: %s",
                                         status_results.size,
                                         mailbox.to_string(),
-                                        response.to_string());
+                                        cmd.status.to_string());
         }
-
         return status_results[0];
     }
 
-    private async StatusResponse send_command_async(ClientSession session,
-                                                    Command cmd,
-                                                    Gee.List<MailboxInformation>? list_results,
-                                                    Gee.List<StatusData>? status_results,
-        Cancellable? cancellable) throws Error {
+    private async void send_command_async(ClientSession session,
+                                          Command cmd,
+                                          Gee.List<MailboxInformation>? list_results,
+                                          Gee.List<StatusData>? status_results,
+                                          GLib.Cancellable? cancellable)
+        throws GLib.Error {
         Gee.Map<Command, StatusResponse> responses = yield send_multiple_async(
             session,
             Geary.iterate<Command>(cmd).to_array_list(),
@@ -413,7 +390,8 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject {
                 "No status response received from server"
             );
         }
-        return response;
+
+        cmd.throw_on_error();
     }
 
     private async Gee.Map<Command, StatusResponse>
diff --git a/src/engine/imap/api/imap-folder-session.vala b/src/engine/imap/api/imap-folder-session.vala
index c94da4bc..1963468e 100644
--- a/src/engine/imap/api/imap-folder-session.vala
+++ b/src/engine/imap/api/imap-folder-session.vala
@@ -113,10 +113,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
         session.status_response_received.connect(on_status_response);
 
         MailboxSpecifier mailbox = session.get_mailbox_for_path(folder.path);
-        StatusResponse? response = yield session.select_async(
-            mailbox, cancellable
-        );
-        throw_on_not_ok(response, "SELECT " + this.folder.path.to_string());
+        yield session.select_async(mailbox, cancellable);
 
         // if at end of SELECT command accepts_user_flags is still
         // UNKKNOWN, treat as TRUE because, according to IMAP spec, if
@@ -319,7 +316,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
         }
 
         foreach (Command cmd in responses.keys) {
-            throw_on_not_ok(responses.get(cmd), cmd.to_string());
+            cmd.throw_on_error();
         }
 
         return responses;
@@ -1130,26 +1127,6 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject {
         return false;
      }
 
-    private void throw_on_not_ok(StatusResponse response, string cmd)
-        throws ImapError {
-        switch (response.status) {
-        case Status.OK:
-            // All good
-            break;
-
-        case Status.NO:
-            throw new ImapError.NOT_SUPPORTED(
-                "Request %s failed: %s", cmd.to_string(), response.to_string()
-            );
-
-        default:
-            throw new ImapError.SERVER_ERROR(
-                "Unknown response status to %s: %s",
-                cmd.to_string(), response.to_string()
-            );
-        }
-    }
-
     private static bool required_but_not_set(Geary.Email.Field check, Geary.Email.Field users_fields, 
Geary.Email email) {
         return users_fields.require(check) ? !email.fields.is_all_set(check) : false;
     }
diff --git a/src/engine/imap/transport/imap-client-session.vala 
b/src/engine/imap/transport/imap-client-session.vala
index fabee175..27dcd9ee 100644
--- a/src/engine/imap/transport/imap-client-session.vala
+++ b/src/engine/imap/transport/imap-client-session.vala
@@ -921,40 +921,12 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
         // should always proceed; only an Error could change this
         assert(params.proceed);
 
-        StatusResponse response = yield command_transaction_async(
-            cmd, cancellable
-        );
-
-        if (response.status != Status.OK) {
-            // Throw an error indicating auth failed here, unless
-            // there is a status response and it indicates that the
-            // server is merely reporting login as being unavailable,
-            // then don't since the creds might actually be fine.
-            ResponseCode? code = response.response_code;
-            if (code != null) {
-                ResponseCodeType? code_type = code.get_response_code_type();
-                if (code_type != null) {
-                    switch (code_type.value) {
-                    case ResponseCodeType.UNAVAILABLE:
-                        throw new ImapError.UNAVAILABLE(
-                            "Login restricted: %s: ", response.to_string()
-                        );
-
-                    case ResponseCodeType.AUTHENTICATIONFAILED:
-                        // pass through to the error being thrown below
-                        break;
-
-                    default:
-                        throw new ImapError.SERVER_ERROR(
-                            "Login error: %s: ", response.to_string()
-                        );
-                    }
-                }
-            }
-
-            throw new ImapError.UNAUTHENTICATED(
-                "Bad credentials: %s: ", response.to_string()
-            );
+        yield command_transaction_async(cmd, cancellable);
+        try {
+            cmd.throw_on_error();
+        } catch (ImapError.OPERATIONAL_ERROR err) {
+            // Catch any plain errors because of a NO
+            throw new ImapError.UNAUTHENTICATED(err.message);
         }
 
         return cmd.status;
@@ -1368,21 +1340,21 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
     // select/examine
     //
 
-    public async StatusResponse select_async(MailboxSpecifier mailbox,
-                                             GLib.Cancellable? cancellable)
+    public async void select_async(MailboxSpecifier mailbox,
+                                   GLib.Cancellable? cancellable)
         throws GLib.Error {
-        return yield select_examine_async(mailbox, true, cancellable);
+        yield select_examine_async(mailbox, true, cancellable);
     }
 
-    public async StatusResponse examine_async(MailboxSpecifier mailbox,
-                                              GLib.Cancellable? cancellable)
+    public async void examine_async(MailboxSpecifier mailbox,
+                                    GLib.Cancellable? cancellable)
         throws GLib.Error {
-        return yield select_examine_async(mailbox, false, cancellable);
+        yield select_examine_async(mailbox, false, cancellable);
     }
 
-    public async StatusResponse select_examine_async(MailboxSpecifier mailbox,
-                                                     bool is_select,
-                                                     GLib.Cancellable? cancellable)
+    public async void select_examine_async(MailboxSpecifier mailbox,
+                                           bool is_select,
+                                           GLib.Cancellable? cancellable)
         throws GLib.Error {
         // Ternary troubles
         Command cmd;
@@ -1399,7 +1371,8 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
 
         assert(params.proceed);
 
-        return yield command_transaction_async(cmd, cancellable);
+        yield command_transaction_async(cmd, cancellable);
+        cmd.throw_on_error();
     }
 
     private uint on_select(uint state, uint event, void *user, Object? object) {
@@ -1454,7 +1427,7 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
     // close mailbox
     //
 
-    public async StatusResponse close_mailbox_async(GLib.Cancellable? cancellable)
+    public async void close_mailbox_async(GLib.Cancellable? cancellable)
         throws GLib.Error {
         CloseCommand cmd = new CloseCommand();
 
@@ -1464,7 +1437,8 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
         if (params.err != null)
             throw params.err;
 
-        return yield command_transaction_async(cmd, cancellable);
+        yield command_transaction_async(cmd, cancellable);
+        cmd.throw_on_error();
     }
 
     private uint on_close_mailbox(uint state, uint event, void *user, Object? object) {