[geary/mjog/imap-connection-fixes: 107/110] Update Geary.Imap.ClientSession namespace handling

[Michael Gratton <mjog src gnome org>]

commit 131e44cb002510bf9efca32757536d8212acee4a
Author: Michael Gratton <mike vee net>
Date:   Mon Dec 30 09:31:31 2019 +1030

    Update Geary.Imap.ClientSession namespace handling
    
    Add namespace accessors, handle updating namespaces when processing
    the received data, not when executing the command so that unsolicited
    namespaces are still recognised. Clear namespaces when new data is
    receieved and when not in selected and auth states. Add unit tests.

 src/engine/imap/api/imap-account-session.vala      |   5 +-
 src/engine/imap/transport/imap-client-session.vala | 131 ++++++++++++++++-----
 .../imap/transport/imap-client-session-test.vala   |  72 +++++++++++
 3 files changed, 175 insertions(+), 33 deletions(-)
---
diff --git a/src/engine/imap/api/imap-account-session.vala b/src/engine/imap/api/imap-account-session.vala
index 4f7f99d6..f0bdfef1 100644
--- a/src/engine/imap/api/imap-account-session.vala
+++ b/src/engine/imap/api/imap-account-session.vala
@@ -46,11 +46,12 @@ internal class Geary.Imap.AccountSession : Geary.Imap.SessionObject {
     public async FolderPath get_default_personal_namespace(Cancellable? cancellable)
     throws Error {
         ClientSession session = claim_session();
-        if (session.personal_namespaces.is_empty) {
+        Gee.List<Namespace> personal = session.get_personal_namespaces();
+        if (personal.is_empty) {
             throw new ImapError.INVALID("No personal namespace found");
         }
 
-        Namespace ns = session.personal_namespaces[0];
+        Namespace ns = personal[0];
         string prefix = ns.prefix;
         string? delim = ns.delim;
         if (delim != null && prefix.has_suffix(delim)) {
diff --git a/src/engine/imap/transport/imap-client-session.vala 
b/src/engine/imap/transport/imap-client-session.vala
index f5f8702a..3c01a409 100644
--- a/src/engine/imap/transport/imap-client-session.vala
+++ b/src/engine/imap/transport/imap-client-session.vala
@@ -271,16 +271,16 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
     // initiate_session_async() has successfully completed.
 
     /** Records the actual name and delimiter used for the inbox */
-    internal MailboxInformation? inbox = null;
+    internal MailboxInformation? inbox { get; private set; default = null; }
 
-    /** The locations personal mailboxes on this  connection. */
-    internal Gee.List<Namespace> personal_namespaces = new Gee.ArrayList<Namespace>();
+    // Locations personal mailboxes for this session
+    private Gee.List<Namespace> personal_namespaces = new Gee.ArrayList<Namespace>();
 
-    /** The locations of other user's mailboxes on this connection. */
-    internal Gee.List<Namespace> user_namespaces = new Gee.ArrayList<Namespace>();
+    // Locations of other user's mailboxes for this session
+    private Gee.List<Namespace> user_namespaces = new Gee.ArrayList<Namespace>();
 
-    /** The locations of shared mailboxes on this connection. */
-    internal Gee.List<Namespace> shared_namespaces = new Gee.ArrayList<Namespace>();
+    // The locations of shared mailboxes for this sesion
+    private Gee.List<Namespace> shared_namespaces = new Gee.ArrayList<Namespace>();
 
     private Endpoint imap_endpoint;
     private Geary.State.Machine fsm;
@@ -339,8 +339,6 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
 
     public signal void status(StatusData status_data);
 
-    public signal void @namespace(NamespaceResponse namespace);
-
     /**
      * If the mailbox name is null it indicates the type of state change that has occurred
      * (authorized -> selected/examined or vice-versa).  If new_name is null readonly should be
@@ -488,6 +486,65 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
         }
     }
 
+    /**
+     * Returns the list of known personal mailbox namespaces.
+     *
+     * A personal namespace is a common prefix of a set of mailboxes
+     * that belong to the currently authenticated account. It may
+     * contain the account's Inbox and Sent mailboxes, for example.
+     *
+     * The list will be empty when the session is not in the
+     * authenticated or selected states, it will contain at least one
+     * after having successfully logged in.
+     *
+     * See [[https://tools.ietf.org/html/rfc2342|RFC 2342]] for more
+     * information.
+     */
+    public Gee.List<Namespace> get_personal_namespaces() {
+        return this.personal_namespaces.read_only_view;
+    }
+
+    /**
+     * Returns the list of known shared mailbox namespaces.
+     *
+     * A shared namespace is a common prefix of a set of mailboxes
+     * that are normally accessible by multiple accounts on the
+     * server, for example shared email mailboxes and NNTP news
+     * mailboxes.
+     *
+     * The list will be empty when the session is not in the
+     * authenticated or selected states, it will only be non-empty
+     * after having successfully logged in and if the server supports
+     * shared mailboxes.
+     *
+     * See [[https://tools.ietf.org/html/rfc2342|RFC 2342]] for more
+     * information.
+     */
+    public Gee.List<Namespace> get_shared_namespaces() {
+        return this.shared_namespaces.read_only_view;
+    }
+
+    /**
+     * Returns the list of known other-users mailbox namespaces.
+     *
+     * An other-user namespace is a common prefix of a set of
+     * mailboxes that are the personal mailboxes of other accounts on
+     * the server. These would not normally be accessible to the
+     * currently authenticated account unless the account has
+     * administration privileges.
+     *
+     * The list will be empty when the session is not in the
+     * authenticated or selected states, it will only be non-empty
+     * after having successfully logged in and if the server or
+     * account supports accessing other account's mailboxes.
+     *
+     * See [[https://tools.ietf.org/html/rfc2342|RFC 2342]] for more
+     * information.
+     */
+    public Gee.List<Namespace> get_other_users_namespaces() {
+        return this.user_namespaces.read_only_view;
+    }
+
     public MailboxSpecifier? get_current_mailbox() {
         return current_mailbox;
     }
@@ -993,18 +1050,12 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
             }
 
             // Try to determine what the connection's namespaces are
-            server_data.clear();
             if (this.capabilities.has_capability(Capabilities.NAMESPACE)) {
                 response = yield send_command_async(
                     new NamespaceCommand(),
                     cancellable
                 );
-                if (response.status == Status.OK && !server_data.is_empty) {
-                    NamespaceResponse ns = server_data[0].get_namespace();
-                    update_namespaces(ns.personal, this.personal_namespaces);
-                    update_namespaces(ns.user, this.user_namespaces);
-                    update_namespaces(ns.shared, this.shared_namespaces);
-                } else {
+                if (response.status != Status.OK) {
                     warning("NAMESPACE command failed");
                 }
             }
@@ -1049,20 +1100,6 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
         }
     }
 
-    private inline void update_namespaces(Gee.List<Namespace>? response, Gee.List<Namespace> list) {
-        if (response != null) {
-            foreach (Namespace ns in response) {
-                list.add(ns);
-                string prefix = ns.prefix;
-                string? delim = ns.delim;
-                if (delim != null && prefix.has_suffix(delim)) {
-                    prefix = prefix.substring(0, prefix.length - delim.length);
-                }
-                this.namespaces.set(prefix, ns);
-            }
-        }
-    }
-
     private uint on_login(uint state, uint event, void *user, Object? object) {
         MachineParams params = (MachineParams) object;
 
@@ -1566,6 +1603,9 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
         uint new_state = state;
         if (validate_state_change_cmd(completion_response)) {
             new_state = State.CLOSED;
+
+            // Namespaces are only valid in AUTH and SELECTED states
+            clear_namespaces();
         }
         return new_state;
     }
@@ -1881,7 +1921,14 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
             break;
 
             case ServerDataType.NAMESPACE:
-                namespace(server_data.get_namespace());
+                // Clear namespaces before updating them, since if
+                // they have changed the new ones take full
+                // precedence.
+                clear_namespaces();
+                NamespaceResponse ns = server_data.get_namespace();
+                update_namespaces(ns.personal, this.personal_namespaces);
+                update_namespaces(ns.shared, this.shared_namespaces);
+                update_namespaces(ns.user, this.user_namespaces);
             break;
 
             // TODO: LSUB
@@ -1896,6 +1943,28 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source {
         server_data_received(server_data);
     }
 
+    private void clear_namespaces() {
+        this.namespaces.clear();
+        this.personal_namespaces.clear();
+        this.shared_namespaces.clear();
+        this.user_namespaces.clear();
+    }
+
+    private void update_namespaces(Gee.List<Namespace>? response,
+                                   Gee.List<Namespace> list) {
+        if (response != null) {
+            foreach (Namespace ns in response) {
+                list.add(ns);
+                string prefix = ns.prefix;
+                string? delim = ns.delim;
+                if (delim != null && prefix.has_suffix(delim)) {
+                    prefix = prefix.substring(0, prefix.length - delim.length);
+                }
+                this.namespaces.set(prefix, ns);
+            }
+        }
+    }
+
     private void on_received_server_data(ServerData server_data) {
         this.last_seen = GLib.get_real_time();
 
diff --git a/test/engine/imap/transport/imap-client-session-test.vala 
b/test/engine/imap/transport/imap-client-session-test.vala
index 8fb304b9..b3243c8f 100644
--- a/test/engine/imap/transport/imap-client-session-test.vala
+++ b/test/engine/imap/transport/imap-client-session-test.vala
@@ -25,6 +25,7 @@ class Geary.Imap.ClientSessionTest : TestCase {
         add_test("login_logout", login_logout);
         add_test("initiate_request_capabilities", initiate_request_capabilities);
         add_test("initiate_implicit_capabilities", initiate_implicit_capabilities);
+        add_test("initiate_namespace", initiate_namespace);
     }
 
     protected override void set_up() throws GLib.Error {
@@ -330,6 +331,77 @@ class Geary.Imap.ClientSessionTest : TestCase {
         );
     }
 
+    public void initiate_namespace() throws GLib.Error {
+        this.server.add_script_line(
+            SEND_LINE,
+            "* OK [CAPABILITY IMAP4rev1 LOGIN] localhost test server ready"
+        );
+        this.server.add_script_line(
+            RECEIVE_LINE, "a001 login test password"
+        );
+        this.server.add_script_line(
+            SEND_LINE, "a001 OK [CAPABILITY IMAP4rev1 NAMESPACE] ohhai"
+        );
+        this.server.add_script_line(
+            RECEIVE_LINE, "a002 LIST \"\" INBOX"
+        );
+        this.server.add_script_line(
+            SEND_LINE, "* LIST (\\HasChildren) \".\" Inbox"
+        );
+        this.server.add_script_line(
+            SEND_LINE, "a002 OK there"
+        );
+        this.server.add_script_line(
+            RECEIVE_LINE, "a003 NAMESPACE"
+        );
+        this.server.add_script_line(
+            SEND_LINE,
+            """* NAMESPACE (("INBOX." ".")) (("user." ".")) (("shared." "."))"""
+        );
+        this.server.add_script_line(SEND_LINE, "a003 OK there");
+        this.server.add_script_line(WAIT_FOR_DISCONNECT, "");
+
+        var test_article = new ClientSession(new_endpoint());
+        assert_true(test_article.get_protocol_state(null) == NOT_CONNECTED);
+
+        test_article.connect_async.begin(
+            CONNECT_TIMEOUT, null, this.async_complete_full
+        );
+        test_article.connect_async.end(async_result());
+        assert_true(test_article.get_protocol_state(null) == UNAUTHORIZED);
+
+        test_article.initiate_session_async.begin(
+            new Credentials(PASSWORD, "test", "password"),
+            null,
+            this.async_complete_full
+        );
+        test_article.initiate_session_async.end(async_result());
+
+        assert_int(1, test_article.get_personal_namespaces().size);
+        assert_string(
+            "INBOX.", test_article.get_personal_namespaces()[0].prefix
+        );
+
+        assert_int(1, test_article.get_shared_namespaces().size);
+        assert_string(
+            "shared.", test_article.get_shared_namespaces()[0].prefix
+        );
+
+        assert_int(1, test_article.get_other_users_namespaces().size);
+        assert_string(
+            "user.", test_article.get_other_users_namespaces()[0].prefix
+        );
+
+        test_article.disconnect_async.begin(null, this.async_complete_full);
+        test_article.disconnect_async.end(async_result());
+
+        TestServer.Result result = this.server.wait_for_script(this.main_loop);
+        assert_true(
+            result.succeeded,
+            result.error != null ? result.error.message : "Server result failed"
+        );
+    }
+
     protected Endpoint new_endpoint() {
         return new Endpoint(this.server.get_client_address(), NONE, 10);
     }