LibIMAP+Userland: Convert LibIMAP::Client to the Serenity Stream APIs

You now cannot get an unconnected LibIMAP::Client, but you can still
close it. This makes for a nicer API where we don't have a Client object
in a limbo state between being constructed and being connected.

This code still isn't as nice as it should be, as TLS::TLSv12 is still
not a Core::Stream::Socket subclass, which would allow for consolidating
most of the TLS/non-TLS code into a single implementation.

Userland/Applications/Mail/MailWidget.cpp

@@ -126,12 +126,15 @@ bool MailWidget::connect_and_login()
             return false;
     }
 
-    m_imap_client = make<IMAP::Client>(server, port, tls);
+    auto maybe_imap_client = tls ? IMAP::Client::connect_tls(server, port) : IMAP::Client::connect_plaintext(server, port);
-    auto connection_promise = m_imap_client->connect();
+    if (maybe_imap_client.is_error()) {
-    if (!connection_promise) {
+        GUI::MessageBox::show_error(window(), String::formatted("Failed to connect to '{}:{}' over {}: {}", server, port, tls ? "TLS" : "Plaintext", maybe_imap_client.error()));
-        GUI::MessageBox::show_error(window(), String::formatted("Failed to connect to '{}:{}' over {}.", server, port, tls ? "TLS" : "Plaintext"));
         return false;
     }
+    m_imap_client = maybe_imap_client.release_value();
+
+    auto connection_promise = m_imap_client->connection_promise();
+    VERIFY(!connection_promise.is_null());
     connection_promise->await();
 
     auto response = m_imap_client->login(username, password)->await().release_value();

Userland/Libraries/LibIMAP/Client.cpp

@@ -4,107 +4,143 @@
  * SPDX-License-Identifier: BSD-2-Clause
  */
 
+#include "AK/OwnPtr.h"
+#include <LibCore/Stream.h>
 #include <LibIMAP/Client.h>
 
 namespace IMAP {
-Client::Client(StringView host, unsigned int port, bool start_with_tls)
+
+Client::Client(StringView host, u16 port, NonnullRefPtr<TLS::TLSv12> socket)
     : m_host(host)
     , m_port(port)
-    , m_tls(start_with_tls)
+    , m_tls(true)
-    , m_parser(Parser())
+    , m_tls_socket(move(socket))
+    , m_connect_pending(Promise<Empty>::construct())
 {
-    if (start_with_tls) {
+    setup_callbacks();
-        m_tls_socket = TLS::TLSv12::construct(nullptr);
-        m_tls_socket->set_root_certificates(DefaultRootCACertificates::the().certificates());
-    } else {
-        m_socket = Core::TCPSocket::construct();
-    }
 }
 
-RefPtr<Promise<Empty>> Client::connect()
+Client::Client(StringView host, u16 port, NonnullOwnPtr<Core::Stream::Socket> socket)
+    : m_host(host)
+    , m_port(port)
+    , m_tls(false)
+    , m_socket(move(socket))
+    , m_connect_pending(Promise<Empty>::construct())
+{
+    setup_callbacks();
+}
+
+Client::Client(Client&& other)
+    : m_host(other.m_host)
+    , m_port(other.m_port)
+    , m_tls(other.m_tls)
+    , m_socket(move(other.m_socket))
+    , m_tls_socket(move(other.m_tls_socket))
+    , m_connect_pending(move(other.m_connect_pending))
+{
+    setup_callbacks();
+}
+
+void Client::setup_callbacks()
 {
-    bool success;
     if (m_tls) {
-        success = connect_tls();
+        m_tls_socket->on_tls_ready_to_read = [&](TLS::TLSv12&) {
+            auto maybe_error = on_tls_ready_to_receive();
+            if (maybe_error.is_error()) {
+                dbgln("Error receiving from the socket: {}", maybe_error.error());
+                close();
+            }
+        };
+
     } else {
-        success = connect_plaintext();
+        m_socket->on_ready_to_read = [&] {
+            auto maybe_error = on_ready_to_receive();
+            if (maybe_error.is_error()) {
+                dbgln("Error receiving from the socket: {}", maybe_error.error());
+                close();
+            }
+        };
     }
-    if (!success)
-        return {};
-    m_connect_pending = Promise<Empty>::construct();
-    return m_connect_pending;
 }
 
-bool Client::connect_tls()
+ErrorOr<NonnullOwnPtr<Client>> Client::connect_tls(StringView host, u16 port)
 {
-    m_tls_socket->on_tls_ready_to_read = [&](TLS::TLSv12&) {
+    auto tls_socket = TLS::TLSv12::construct(nullptr);
-        on_tls_ready_to_receive();
+    tls_socket->set_root_certificates(DefaultRootCACertificates::the().certificates());
-    };
+
-    m_tls_socket->on_tls_error = [&](TLS::AlertDescription alert) {
+    tls_socket->on_tls_error = [&](TLS::AlertDescription alert) {
         dbgln("failed: {}", alert_name(alert));
     };
-    m_tls_socket->on_tls_connected = [&] {
+    tls_socket->on_tls_connected = [&] {
         dbgln("connected");
     };
-    auto success = m_tls_socket->connect(m_host, m_port);
+
-    dbgln("connecting to {}:{} {}", m_host, m_port, success);
+    auto success = tls_socket->connect(host, port);
-    return success;
+    dbgln("connecting to {}:{} {}", host, port, success);
+
+    return adopt_nonnull_own_or_enomem(new (nothrow) Client(host, port, tls_socket));
 }
 
-bool Client::connect_plaintext()
+ErrorOr<NonnullOwnPtr<Client>> Client::connect_plaintext(StringView host, u16 port)
 {
-    m_socket->on_ready_to_read = [&] {
+    auto socket = TRY(Core::Stream::TCPSocket::connect(host, port));
-        on_ready_to_receive();
+    dbgln("Connected to {}:{}", host, port);
-    };
+    return adopt_nonnull_own_or_enomem(new (nothrow) Client(host, port, move(socket)));
-    auto success = m_socket->connect(m_host, m_port);
-    dbgln("connecting to {}:{} {}", m_host, m_port, success);
-    return success;
 }
 
-void Client::on_tls_ready_to_receive()
+ErrorOr<void> Client::on_tls_ready_to_receive()
 {
     if (!m_tls_socket->can_read())
-        return;
+        return {};
     auto data = m_tls_socket->read();
+    // FIXME: Make TLSv12 return the actual error instead of returning a bogus
+    //        one here.
     if (!data.has_value())
-        return;
+        return Error::from_errno(EIO);
 
     // Once we get server hello we can start sending
     if (m_connect_pending) {
         m_connect_pending->resolve({});
         m_connect_pending.clear();
-        return;
+        return {};
     }
 
     m_buffer += data.value();
     if (m_buffer[m_buffer.size() - 1] == '\n') {
         // Don't try parsing until we have a complete line.
         auto response = m_parser.parse(move(m_buffer), m_expecting_response);
-        handle_parsed_response(move(response));
+        MUST(handle_parsed_response(move(response)));
         m_buffer.clear();
     }
+
+    return {};
 }
 
-void Client::on_ready_to_receive()
+ErrorOr<void> Client::on_ready_to_receive()
 {
-    if (!m_socket->can_read())
+    if (!TRY(m_socket->can_read_without_blocking()))
-        return;
+        return {};
-    m_buffer += m_socket->read_all();
+
+    auto pending_bytes = TRY(m_socket->pending_bytes());
+    auto receive_buffer = TRY(m_buffer.get_bytes_for_writing(pending_bytes));
+    TRY(m_socket->read(receive_buffer));
 
     // Once we get server hello we can start sending.
     if (m_connect_pending) {
         m_connect_pending->resolve({});
         m_connect_pending.clear();
         m_buffer.clear();
-        return;
+        return {};
     }
 
     if (m_buffer[m_buffer.size() - 1] == '\n') {
         // Don't try parsing until we have a complete line.
         auto response = m_parser.parse(move(m_buffer), m_expecting_response);
-        handle_parsed_response(move(response));
+        TRY(handle_parsed_response(move(response)));
         m_buffer.clear();
     }
+
+    return {};
 }
 
 static ReadonlyBytes command_byte_buffer(CommandType command)
@@ -170,15 +206,17 @@ static ReadonlyBytes command_byte_buffer(CommandType command)
     VERIFY_NOT_REACHED();
 }
 
-void Client::send_raw(StringView data)
+ErrorOr<void> Client::send_raw(StringView data)
 {
     if (m_tls) {
         m_tls_socket->write(data.bytes());
         m_tls_socket->write("\r\n"sv.bytes());
     } else {
-        m_socket->write(data.bytes());
+        TRY(m_socket->write(data.bytes()));
-        m_socket->write("\r\n"sv.bytes());
+        TRY(m_socket->write("\r\n"sv.bytes()));
     }
+
+    return {};
 }
 
 RefPtr<Promise<Optional<Response>>> Client::send_command(Command&& command)
@@ -190,7 +228,7 @@ RefPtr<Promise<Optional<Response>>> Client::send_command(Command&& command)
     m_pending_promises.append(promise);
 
     if (m_pending_promises.size() == 1)
-        send_next_command();
+        MUST(send_next_command());
 
     return promise;
 }
@@ -245,7 +283,7 @@ RefPtr<Promise<Optional<SolidResponse>>> Client::select(StringView string)
     return cast_promise<SolidResponse>(send_command(move(command)));
 }
 
-void Client::handle_parsed_response(ParseStatus&& parse_status)
+ErrorOr<void> Client::handle_parsed_response(ParseStatus&& parse_status)
 {
     if (!m_expecting_response) {
         if (!parse_status.successful) {
@@ -268,12 +306,14 @@ void Client::handle_parsed_response(ParseStatus&& parse_status)
         }
 
         if (should_send_next && !m_command_queue.is_empty()) {
-            send_next_command();
+            TRY(send_next_command());
         }
     }
+
+    return {};
 }
 
-void Client::send_next_command()
+ErrorOr<void> Client::send_next_command()
 {
     auto command = m_command_queue.take_first();
     ByteBuffer buffer;
@@ -287,8 +327,9 @@ void Client::send_next_command()
         buffer.append(arg.bytes().data(), arg.length());
     }
 
-    send_raw(buffer);
+    TRY(send_raw(buffer));
     m_expecting_response = true;
+    return {};
 }
 
 RefPtr<Promise<Optional<SolidResponse>>> Client::examine(StringView string)
@@ -358,7 +399,7 @@ RefPtr<Promise<Optional<SolidResponse>>> Client::finish_idle()
 {
     auto promise = Promise<Optional<Response>>::construct();
     m_pending_promises.append(promise);
-    send_raw("DONE");
+    MUST(send_raw("DONE"));
     m_expecting_response = true;
     return cast_promise<SolidResponse>(promise);
 }
@@ -415,9 +456,9 @@ RefPtr<Promise<Optional<SolidResponse>>> Client::append(StringView mailbox, Mess
 
     continue_req->on_resolved = [this, message2 { move(message) }](auto& data) {
         if (!data.has_value()) {
-            handle_parsed_response({ .successful = false, .response = {} });
+            MUST(handle_parsed_response({ .successful = false, .response = {} }));
         } else {
-            send_raw(message2.data);
+            MUST(send_raw(message2.data));
             m_expecting_response = true;
         }
     };
@@ -452,6 +493,7 @@ RefPtr<Promise<Optional<SolidResponse>>> Client::copy(Sequence sequence_set, Str
 
     return cast_promise<SolidResponse>(send_command(move(command)));
 }
+
 void Client::close()
 {
     if (m_tls) {
@@ -460,4 +502,10 @@ void Client::close()
         m_socket->close();
     }
 }
+
+bool Client::is_open()
+{
+    return m_tls ? m_tls_socket->is_open() : m_socket->is_open();
+}
+
 }

Userland/Libraries/LibIMAP/Client.h

@@ -8,6 +8,7 @@
 
 #include <AK/Function.h>
 #include <LibCore/Promise.h>
+#include <LibCore/Stream.h>
 #include <LibIMAP/Parser.h>
 #include <LibTLS/TLSv12.h>
 
@@ -16,15 +17,23 @@ template<typename T>
 using Promise = Core::Promise<T>;
 
 class Client {
+    AK_MAKE_NONCOPYABLE(Client);
     friend class Parser;
 
 public:
-    Client(StringView host, unsigned port, bool start_with_tls);
+    static ErrorOr<NonnullOwnPtr<Client>> connect_tls(StringView host, u16 port);
+    static ErrorOr<NonnullOwnPtr<Client>> connect_plaintext(StringView host, u16 port);
+
+    Client(Client&&);
+
+    RefPtr<Promise<Empty>> connection_promise()
+    {
+        return m_connect_pending;
+    }
 
-    RefPtr<Promise<Empty>> connect();
     RefPtr<Promise<Optional<Response>>> send_command(Command&&);
     RefPtr<Promise<Optional<Response>>> send_simple_command(CommandType);
-    void send_raw(StringView data);
+    ErrorOr<void> send_raw(StringView data);
     RefPtr<Promise<Optional<SolidResponse>>> login(StringView username, StringView password);
     RefPtr<Promise<Optional<SolidResponse>>> list(StringView reference_name, StringView mailbox_name);
     RefPtr<Promise<Optional<SolidResponse>>> lsub(StringView reference_name, StringView mailbox_name);
@@ -45,37 +54,42 @@ public:
     RefPtr<Promise<Optional<SolidResponse>>> status(StringView mailbox, Vector<StatusItemType> const& types);
     RefPtr<Promise<Optional<SolidResponse>>> append(StringView mailbox, Message&& message, Optional<Vector<String>> flags = {}, Optional<Core::DateTime> date_time = {});
 
+    bool is_open();
     void close();
 
     Function<void(ResponseData&&)> unrequested_response_callback;
 
 private:
-    StringView m_host;
+    Client(StringView host, u16 port, NonnullRefPtr<TLS::TLSv12>);
-    unsigned m_port;
+    Client(StringView host, u16 port, NonnullOwnPtr<Core::Stream::Socket>);
-    RefPtr<Core::Socket> m_socket;
+    void setup_callbacks();
-    RefPtr<TLS::TLSv12> m_tls_socket;
 
-    void on_ready_to_receive();
+    ErrorOr<void> on_ready_to_receive();
-    void on_tls_ready_to_receive();
+    ErrorOr<void> on_tls_ready_to_receive();
+
+    ErrorOr<void> handle_parsed_response(ParseStatus&& parse_status);
+    ErrorOr<void> send_next_command();
+
+    StringView m_host;
+    u16 m_port;
 
     bool m_tls;
-    int m_current_command = 1;
+    // FIXME: Convert this to a single `NonnullOwnPtr<Core::Stream::Socket>`
+    //        once `TLS::TLSv12` is converted to a `Socket` as well.
+    OwnPtr<Core::Stream::Socket> m_socket;
+    RefPtr<TLS::TLSv12> m_tls_socket;
+    RefPtr<Promise<Empty>> m_connect_pending {};
 
-    bool connect_tls();
+    int m_current_command = 1;
-    bool connect_plaintext();
 
     // Sent but response not received
     Vector<RefPtr<Promise<Optional<Response>>>> m_pending_promises;
     // Not yet sent
     Vector<Command> m_command_queue {};
 
-    RefPtr<Promise<Empty>> m_connect_pending {};
-
     ByteBuffer m_buffer;
-    Parser m_parser;
+    Parser m_parser {};
 
     bool m_expecting_response { false };
-    void handle_parsed_response(ParseStatus&& parse_status);
-    void send_next_command();
 };
 }

Userland/Utilities/test-imap.cpp

@@ -43,21 +43,21 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
     }
 
     Core::EventLoop loop;
-    auto client = IMAP::Client(host, port, tls);
+    auto client = TRY(tls ? IMAP::Client::connect_tls(host, port) : IMAP::Client::connect_plaintext(host, port));
-    client.connect()->await();
+    client->connection_promise()->await();
 
-    auto response = client.login(username, password.view())->await().release_value();
+    auto response = client->login(username, password.view())->await().release_value();
     outln("[LOGIN] Login response: {}", response.response_text());
 
-    response = move(client.send_simple_command(IMAP::CommandType::Capability)->await().value().get<IMAP::SolidResponse>());
+    response = move(client->send_simple_command(IMAP::CommandType::Capability)->await().value().get<IMAP::SolidResponse>());
     outln("[CAPABILITY] First capability: {}", response.data().capabilities().first());
     bool idle_supported = !response.data().capabilities().find_if([](auto capability) { return capability.equals_ignoring_case("IDLE"); }).is_end();
 
-    response = client.list("", "*")->await().release_value();
+    response = client->list("", "*")->await().release_value();
     outln("[LIST] First mailbox: {}", response.data().list_items().first().name);
 
-    auto mailbox = "Inbox";
+    auto mailbox = "Inbox"sv;
-    response = client.select(mailbox)->await().release_value();
+    response = client->select(mailbox)->await().release_value();
     outln("[SELECT] Select response: {}", response.response_text());
 
     auto message = Message {
@@ -70,7 +70,7 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
         "This is a message just to say hello.\r\n"
         "So, \"Hello\"."
     };
-    auto promise = client.append("INBOX", move(message));
+    auto promise = client->append("INBOX", move(message));
     response = promise->await().release_value();
     outln("[APPEND] Response: {}", response.response_text());
 
@@ -79,13 +79,13 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
         IMAP::SearchKey::From { "jdoe@machine.example" } });
     keys.append(IMAP::SearchKey {
         IMAP::SearchKey::Subject { "Saying Hello" } });
-    response = client.search({}, move(keys), false)->await().release_value();
+    response = client->search({}, move(keys), false)->await().release_value();
 
     Vector<unsigned> search_results = move(response.data().search_results());
-    int added_message = search_results.first();
+    auto added_message = search_results.first();
     outln("[SEARCH] Number of results: {}", search_results.size());
 
-    response = client.status("INBOX", { IMAP::StatusItemType::Recent, IMAP::StatusItemType::Messages })->await().release_value();
+    response = client->status("INBOX", { IMAP::StatusItemType::Recent, IMAP::StatusItemType::Messages })->await().release_value();
     outln("[STATUS] Recent items: {}", response.data().status_item().get(IMAP::StatusItemType::Recent));
 
     for (auto item : search_results) {
@@ -118,7 +118,7 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
         };
         // clang-format on
 
-        auto fetch_response = client.fetch(fetch_command, false)->await().release_value();
+        auto fetch_response = client->fetch(fetch_command, false)->await().release_value();
         outln("[FETCH] Subject of search result: {}",
             fetch_response.data()
                 .fetch_data()
@@ -133,25 +133,28 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
                 .value());
     }
 
-    response = client.store(IMAP::StoreMethod::Add, { added_message, added_message }, false, { "\\Deleted" }, false)->await().release_value();
+    // FIXME: There is a discrepancy between IMAP::Sequence wanting signed ints
+    //        and IMAP search results returning unsigned ones. Find which one is
+    //        more correct and fix this.
+    response = client->store(IMAP::StoreMethod::Add, { static_cast<int>(added_message), static_cast<int>(added_message) }, false, { "\\Deleted" }, false)->await().release_value();
     outln("[STORE] Store response: {}", response.response_text());
 
-    response = move(client.send_simple_command(IMAP::CommandType::Expunge)->await().release_value().get<IMAP::SolidResponse>());
+    response = move(client->send_simple_command(IMAP::CommandType::Expunge)->await().release_value().get<IMAP::SolidResponse>());
     outln("[EXPUNGE] Number of expunged entries: {}", response.data().expunged().size());
 
     if (idle_supported) {
-        VERIFY(client.idle()->await().has_value());
+        VERIFY(client->idle()->await().has_value());
         sleep(3);
-        response = client.finish_idle()->await().release_value();
+        response = client->finish_idle()->await().release_value();
         outln("[IDLE] Idle response: {}", response.response_text());
     } else {
         outln("[IDLE] Skipped. No IDLE support.");
     }
 
-    response = move(client.send_simple_command(IMAP::CommandType::Logout)->await().release_value().get<IMAP::SolidResponse>());
+    response = move(client->send_simple_command(IMAP::CommandType::Logout)->await().release_value().get<IMAP::SolidResponse>());
     outln("[LOGOUT] Bye: {}", response.data().bye_message().value());
 
-    client.close();
+    client->close();
 
     return 0;
 }