From 6128e859acd0673f866ea4898fb9229cd066ee65 Mon Sep 17 00:00:00 2001 From: Valtteri Koskivuori Date: Wed, 26 Jul 2023 22:40:05 +0300 Subject: [PATCH] LibIMAP+Mail: Propagate errors from LibIMAP and MailWidget This lets us bubble up errors from `LibIMAP::Client::send_command()`, which can happen if the connection hangs or is taking a long time, and the user closes Mail. --- Userland/Applications/Mail/MailWidget.cpp | 22 +++--- Userland/Applications/Mail/MailWidget.h | 2 +- Userland/Applications/Mail/main.cpp | 5 +- Userland/Libraries/LibIMAP/Client.cpp | 82 +++++++++++------------ Userland/Libraries/LibIMAP/Client.h | 42 ++++++------ Userland/Utilities/test-imap.cpp | 26 +++---- 6 files changed, 90 insertions(+), 89 deletions(-) diff --git a/Userland/Applications/Mail/MailWidget.cpp b/Userland/Applications/Mail/MailWidget.cpp index 5174e586d65..788cde4c35f 100644 --- a/Userland/Applications/Mail/MailWidget.cpp +++ b/Userland/Applications/Mail/MailWidget.cpp @@ -96,7 +96,7 @@ MailWidget::MailWidget() }; } -bool MailWidget::connect_and_login() +ErrorOr MailWidget::connect_and_login() { auto server = Config::read_string("Mail"sv, "Connection"sv, "Server"sv, {}); @@ -117,6 +117,7 @@ bool MailWidget::connect_and_login() return false; } + // FIXME: Plaintext password storage, yikes! auto password = Config::read_string("Mail"sv, "User"sv, "Password"sv, {}); while (password.is_empty()) { if (GUI::PasswordInputDialog::show(window(), password, "Login"sv, server, username) != GUI::Dialog::ExecResult::OK) @@ -131,13 +132,12 @@ bool MailWidget::connect_and_login() } m_imap_client = maybe_imap_client.release_value(); - auto connection_promise = m_imap_client->connection_promise(); - VERIFY(!connection_promise.is_null()); - MUST(connection_promise->await()); + TRY(m_imap_client->connection_promise()->await()); - auto response = MUST(m_imap_client->login(username, password)->await()).release_value(); m_statusbar->set_text(String::formatted("Connected. Logging in as {}...", username).release_value_but_fixme_should_propagate_errors()); + auto response = TRY(TRY(m_imap_client->login(username, password))->await()).release_value(); + if (response.status() != IMAP::ResponseStatus::OK) { dbgln("Failed to login. The server says: '{}'", response.response_text()); GUI::MessageBox::show_error(window(), DeprecatedString::formatted("Failed to login. The server says: '{}'", response.response_text())); @@ -146,7 +146,7 @@ bool MailWidget::connect_and_login() } m_statusbar->set_text("Logged in. Loading mailboxes..."_string); - response = MUST(m_imap_client->list(""sv, "*"sv)->await()).release_value(); + response = TRY(TRY(m_imap_client->list(""sv, "*"sv))->await()).release_value(); if (response.status() != IMAP::ResponseStatus::OK) { dbgln("Failed to retrieve mailboxes. The server says: '{}'", response.response_text()); @@ -173,7 +173,7 @@ void MailWidget::on_window_close() // User closed main window before a connection was established return; } - auto response = move(MUST(m_imap_client->send_simple_command(IMAP::CommandType::Logout)->await()).release_value().get()); + auto response = move(MUST(MUST(m_imap_client->send_simple_command(IMAP::CommandType::Logout))->await()).release_value().get()); VERIFY(response.status() == IMAP::ResponseStatus::OK); m_imap_client->close(); @@ -263,7 +263,7 @@ void MailWidget::selected_mailbox() if (mailbox.flags & (unsigned)IMAP::MailboxFlag::NoSelect) return; - auto response = MUST(m_imap_client->select(mailbox.name)->await()).release_value(); + auto response = MUST(MUST(m_imap_client->select(mailbox.name))->await()).release_value(); if (response.status() != IMAP::ResponseStatus::OK) { dbgln("Failed to select mailbox. The server says: '{}'", response.response_text()); @@ -292,7 +292,7 @@ void MailWidget::selected_mailbox() }, }; - auto fetch_response = MUST(m_imap_client->fetch(fetch_command, false)->await()).release_value(); + auto fetch_response = MUST(MUST(m_imap_client->fetch(fetch_command, false))->await()).release_value(); if (response.status() != IMAP::ResponseStatus::OK) { dbgln("Failed to retrieve subject/from for e-mails. The server says: '{}'", response.response_text()); @@ -441,7 +441,7 @@ void MailWidget::selected_email_to_load() }, }; - auto fetch_response = MUST(m_imap_client->fetch(fetch_command, false)->await()).release_value(); + auto fetch_response = MUST(MUST(m_imap_client->fetch(fetch_command, false))->await()).release_value(); if (fetch_response.status() != IMAP::ResponseStatus::OK) { dbgln("Failed to retrieve the body structure of the selected e-mail. The server says: '{}'", fetch_response.response_text()); @@ -502,7 +502,7 @@ void MailWidget::selected_email_to_load() }, }; - fetch_response = MUST(m_imap_client->fetch(fetch_command, false)->await()).release_value(); + fetch_response = MUST(MUST(m_imap_client->fetch(fetch_command, false))->await()).release_value(); if (fetch_response.status() != IMAP::ResponseStatus::OK) { dbgln("Failed to retrieve the body of the selected e-mail. The server says: '{}'", fetch_response.response_text()); diff --git a/Userland/Applications/Mail/MailWidget.h b/Userland/Applications/Mail/MailWidget.h index 4ba4836b61e..dc73cb7525e 100644 --- a/Userland/Applications/Mail/MailWidget.h +++ b/Userland/Applications/Mail/MailWidget.h @@ -19,7 +19,7 @@ class MailWidget final : public GUI::Widget { public: virtual ~MailWidget() override = default; - bool connect_and_login(); + ErrorOr connect_and_login(); void on_window_close(); diff --git a/Userland/Applications/Mail/main.cpp b/Userland/Applications/Mail/main.cpp index e27a8b0ddbd..c302fef6191 100644 --- a/Userland/Applications/Mail/main.cpp +++ b/Userland/Applications/Mail/main.cpp @@ -63,9 +63,10 @@ ErrorOr serenity_main(Main::Arguments arguments) window->show(); - bool should_continue = mail_widget->connect_and_login(); - if (!should_continue) + bool should_continue = TRY(mail_widget->connect_and_login()); + if (!should_continue) { return 1; + } return app->exec(); } diff --git a/Userland/Libraries/LibIMAP/Client.cpp b/Userland/Libraries/LibIMAP/Client.cpp index 5bf83f70ad6..4572baa587a 100644 --- a/Userland/Libraries/LibIMAP/Client.cpp +++ b/Userland/Libraries/LibIMAP/Client.cpp @@ -151,7 +151,7 @@ ErrorOr Client::send_raw(StringView data) return {}; } -RefPtr>> Client::send_command(Command&& command) +ErrorOr>>> Client::send_command(Command&& command) { m_command_queue.append(move(command)); m_current_command++; @@ -160,7 +160,7 @@ RefPtr>> Client::send_command(Command&& command) m_pending_promises.append(promise); if (m_pending_promises.size() == 1) - MUST(send_next_command()); + TRY(send_next_command()); return promise; } @@ -175,44 +175,44 @@ RefPtr>> cast_promise(RefPtr>> pr return new_promise; } -RefPtr>> Client::login(StringView username, StringView password) +ErrorOr>>> Client::login(StringView username, StringView password) { auto command = Command { CommandType::Login, m_current_command, { serialize_astring(username), serialize_astring(password) } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } -RefPtr>> Client::list(StringView reference_name, StringView mailbox) +ErrorOr>>> Client::list(StringView reference_name, StringView mailbox) { auto command = Command { CommandType::List, m_current_command, { DeprecatedString::formatted("\"{}\"", reference_name), DeprecatedString::formatted("\"{}\"", mailbox) } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } -RefPtr>> Client::lsub(StringView reference_name, StringView mailbox) +ErrorOr>>> Client::lsub(StringView reference_name, StringView mailbox) { auto command = Command { CommandType::ListSub, m_current_command, { DeprecatedString::formatted("\"{}\"", reference_name), DeprecatedString::formatted("\"{}\"", mailbox) } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } -RefPtr>> Client::fetch(FetchCommand request, bool uid) +ErrorOr>>> Client::fetch(FetchCommand request, bool uid) { auto command = Command { uid ? CommandType::UIDFetch : CommandType::Fetch, m_current_command, { request.serialize() } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } -RefPtr>> Client::send_simple_command(CommandType type) +ErrorOr>>> Client::send_simple_command(CommandType type) { auto command = Command { type, m_current_command, {} }; - return send_command(move(command)); + return TRY(send_command(move(command))); } -RefPtr>> Client::select(StringView string) +ErrorOr>>> Client::select(StringView string) { auto command = Command { CommandType::Select, m_current_command, { serialize_astring(string) } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } ErrorOr Client::handle_parsed_response(ParseStatus&& parse_status) @@ -264,25 +264,25 @@ ErrorOr Client::send_next_command() return {}; } -RefPtr>> Client::examine(StringView string) +ErrorOr>>> Client::examine(StringView string) { auto command = Command { CommandType::Examine, m_current_command, { serialize_astring(string) } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } -RefPtr>> Client::create_mailbox(StringView name) +ErrorOr>>> Client::create_mailbox(StringView name) { auto command = Command { CommandType::Create, m_current_command, { serialize_astring(name) } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } -RefPtr>> Client::delete_mailbox(StringView name) +ErrorOr>>> Client::delete_mailbox(StringView name) { auto command = Command { CommandType::Delete, m_current_command, { serialize_astring(name) } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } -RefPtr>> Client::store(StoreMethod method, Sequence sequence_set, bool silent, Vector const& flags, bool uid) +ErrorOr>>> Client::store(StoreMethod method, Sequence sequence_set, bool silent, Vector const& flags, bool uid) { StringBuilder data_item_name; switch (method) { @@ -306,9 +306,9 @@ RefPtr>> Client::store(StoreMethod method, Seque flags_builder.append(')'); auto command = Command { uid ? CommandType::UIDStore : CommandType::Store, m_current_command, { sequence_set.serialize(), data_item_name.to_deprecated_string(), flags_builder.to_deprecated_string() } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } -RefPtr>> Client::search(Optional charset, Vector&& keys, bool uid) +ErrorOr>>> Client::search(Optional charset, Vector&& keys, bool uid) { Vector args; if (charset.has_value()) { @@ -319,15 +319,15 @@ RefPtr>> Client::search(Optional(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } -RefPtr>> Client::idle() +ErrorOr>>> Client::idle() { - auto promise = send_simple_command(CommandType::Idle); + auto promise = TRY(send_simple_command(CommandType::Idle)); return cast_promise(promise); } -RefPtr>> Client::finish_idle() +ErrorOr>>> Client::finish_idle() { auto promise = Promise>::construct(); m_pending_promises.append(promise); @@ -336,7 +336,7 @@ RefPtr>> Client::finish_idle() return cast_promise(promise); } -RefPtr>> Client::status(StringView mailbox, Vector const& types) +ErrorOr>>> Client::status(StringView mailbox, Vector const& types) { Vector args; for (auto type : types) { @@ -363,10 +363,10 @@ RefPtr>> Client::status(StringView mailbox, Vect types_list.join(' ', args); types_list.append(')'); auto command = Command { CommandType::Status, m_current_command, { mailbox, types_list.to_deprecated_string() } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } -RefPtr>> Client::append(StringView mailbox, Message&& message, Optional> flags, Optional date_time) +ErrorOr>>> Client::append(StringView mailbox, Message&& message, Optional> flags, Optional date_time) { Vector args = { mailbox }; if (flags.has_value()) { @@ -381,7 +381,7 @@ RefPtr>> Client::append(StringView mailbox, Mess args.append(DeprecatedString::formatted("{{{}}}", message.data.length())); - auto continue_req = send_command(Command { CommandType::Append, m_current_command, args }); + auto continue_req = TRY(send_command(Command { CommandType::Append, m_current_command, args })); auto response_promise = Promise>::construct(); m_pending_promises.append(response_promise); @@ -398,33 +398,33 @@ RefPtr>> Client::append(StringView mailbox, Mess return cast_promise(response_promise); } -RefPtr>> Client::subscribe(StringView mailbox) +ErrorOr>>> Client::subscribe(StringView mailbox) { auto command = Command { CommandType::Subscribe, m_current_command, { serialize_astring(mailbox) } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } -RefPtr>> Client::unsubscribe(StringView mailbox) +ErrorOr>>> Client::unsubscribe(StringView mailbox) { auto command = Command { CommandType::Unsubscribe, m_current_command, { serialize_astring(mailbox) } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } -RefPtr>> Client::authenticate(StringView method) +ErrorOr>>> Client::authenticate(StringView method) { auto command = Command { CommandType::Authenticate, m_current_command, { method } }; - return send_command(move(command)); + return TRY(send_command(move(command))); } -RefPtr>> Client::rename(StringView from, StringView to) +ErrorOr>>> Client::rename(StringView from, StringView to) { auto command = Command { CommandType::Rename, m_current_command, { serialize_astring(from), serialize_astring(to) } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } -RefPtr>> Client::copy(Sequence sequence_set, StringView name, bool uid) +ErrorOr>>> Client::copy(Sequence sequence_set, StringView name, bool uid) { auto command = Command { uid ? CommandType::UIDCopy : CommandType::Copy, m_current_command, { sequence_set.serialize(), serialize_astring(name) } }; - return cast_promise(send_command(move(command))); + return cast_promise(TRY(send_command(move(command)))); } void Client::close() diff --git a/Userland/Libraries/LibIMAP/Client.h b/Userland/Libraries/LibIMAP/Client.h index 9f5e6af3890..92f926f75e7 100644 --- a/Userland/Libraries/LibIMAP/Client.h +++ b/Userland/Libraries/LibIMAP/Client.h @@ -30,28 +30,28 @@ public: return m_connect_pending; } - RefPtr>> send_command(Command&&); - RefPtr>> send_simple_command(CommandType); + ErrorOr>>> send_command(Command&&); + ErrorOr>>> send_simple_command(CommandType); ErrorOr send_raw(StringView data); - RefPtr>> login(StringView username, StringView password); - RefPtr>> list(StringView reference_name, StringView mailbox_name); - RefPtr>> lsub(StringView reference_name, StringView mailbox_name); - RefPtr>> select(StringView string); - RefPtr>> examine(StringView string); - RefPtr>> search(Optional charset, Vector&& search_keys, bool uid); - RefPtr>> fetch(FetchCommand request, bool uid); - RefPtr>> store(StoreMethod, Sequence, bool silent, Vector const& flags, bool uid); - RefPtr>> copy(Sequence sequence_set, StringView name, bool uid); - RefPtr>> create_mailbox(StringView name); - RefPtr>> delete_mailbox(StringView name); - RefPtr>> subscribe(StringView mailbox); - RefPtr>> unsubscribe(StringView mailbox); - RefPtr>> rename(StringView from, StringView to); - RefPtr>> authenticate(StringView method); - RefPtr>> idle(); - RefPtr>> finish_idle(); - RefPtr>> status(StringView mailbox, Vector const& types); - RefPtr>> append(StringView mailbox, Message&& message, Optional> flags = {}, Optional date_time = {}); + ErrorOr>>> login(StringView username, StringView password); + ErrorOr>>> list(StringView reference_name, StringView mailbox_name); + ErrorOr>>> lsub(StringView reference_name, StringView mailbox_name); + ErrorOr>>> select(StringView string); + ErrorOr>>> examine(StringView string); + ErrorOr>>> search(Optional charset, Vector&& search_keys, bool uid); + ErrorOr>>> fetch(FetchCommand request, bool uid); + ErrorOr>>> store(StoreMethod, Sequence, bool silent, Vector const& flags, bool uid); + ErrorOr>>> copy(Sequence sequence_set, StringView name, bool uid); + ErrorOr>>> create_mailbox(StringView name); + ErrorOr>>> delete_mailbox(StringView name); + ErrorOr>>> subscribe(StringView mailbox); + ErrorOr>>> unsubscribe(StringView mailbox); + ErrorOr>>> rename(StringView from, StringView to); + ErrorOr>>> authenticate(StringView method); + ErrorOr>>> idle(); + ErrorOr>>> finish_idle(); + ErrorOr>>> status(StringView mailbox, Vector const& types); + ErrorOr>>> append(StringView mailbox, Message&& message, Optional> flags = {}, Optional date_time = {}); bool is_open(); void close(); diff --git a/Userland/Utilities/test-imap.cpp b/Userland/Utilities/test-imap.cpp index 218a2ea6fb7..77db05cdab3 100644 --- a/Userland/Utilities/test-imap.cpp +++ b/Userland/Utilities/test-imap.cpp @@ -46,18 +46,18 @@ ErrorOr serenity_main(Main::Arguments arguments) auto client = TRY(tls ? IMAP::Client::connect_tls(host, port) : IMAP::Client::connect_plaintext(host, port)); TRY(client->connection_promise()->await()); - auto response = TRY(client->login(username, password.view())->await()).release_value(); + auto response = TRY(TRY(client->login(username, password.view()))->await()).release_value(); outln("[LOGIN] Login response: {}", response.response_text()); - response = move(TRY(client->send_simple_command(IMAP::CommandType::Capability)->await()).value().get()); + response = move(TRY(TRY(client->send_simple_command(IMAP::CommandType::Capability))->await()).value().get()); outln("[CAPABILITY] First capability: {}", response.data().capabilities().first()); bool idle_supported = !response.data().capabilities().find_if([](auto capability) { return capability.equals_ignoring_ascii_case("IDLE"sv); }).is_end(); - response = TRY(client->list(""sv, "*"sv)->await()).release_value(); + response = TRY(TRY(client->list(""sv, "*"sv))->await()).release_value(); outln("[LIST] First mailbox: {}", response.data().list_items().first().name); auto mailbox = "Inbox"sv; - response = TRY(client->select(mailbox)->await()).release_value(); + response = TRY(TRY(client->select(mailbox))->await()).release_value(); outln("[SELECT] Select response: {}", response.response_text()); auto message = Message { @@ -70,7 +70,7 @@ ErrorOr serenity_main(Main::Arguments arguments) "This is a message just to say hello.\r\n" "So, \"Hello\"." }; - auto promise = client->append("INBOX"sv, move(message)); + auto promise = TRY(client->append("INBOX"sv, move(message))); response = TRY(promise->await()).release_value(); outln("[APPEND] Response: {}", response.response_text()); @@ -79,13 +79,13 @@ ErrorOr serenity_main(Main::Arguments arguments) IMAP::SearchKey::From { "jdoe@machine.example" } }); keys.append(IMAP::SearchKey { IMAP::SearchKey::Subject { "Saying Hello" } }); - response = TRY(client->search({}, move(keys), false)->await()).release_value(); + response = TRY(TRY(client->search({}, move(keys), false))->await()).release_value(); Vector search_results = move(response.data().search_results()); auto added_message = search_results.first(); outln("[SEARCH] Number of results: {}", search_results.size()); - response = TRY(client->status("INBOX"sv, { IMAP::StatusItemType::Recent, IMAP::StatusItemType::Messages })->await()).release_value(); + response = TRY(TRY(client->status("INBOX"sv, { 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 serenity_main(Main::Arguments arguments) }; // clang-format on - auto fetch_response = TRY(client->fetch(fetch_command, false)->await()).release_value(); + auto fetch_response = TRY(TRY(client->fetch(fetch_command, false))->await()).release_value(); outln("[FETCH] Subject of search result: {}", fetch_response.data() .fetch_data() @@ -136,22 +136,22 @@ ErrorOr serenity_main(Main::Arguments arguments) // 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 = TRY(client->store(IMAP::StoreMethod::Add, { static_cast(added_message), static_cast(added_message) }, false, { "\\Deleted" }, false)->await()).release_value(); + response = TRY(TRY(client->store(IMAP::StoreMethod::Add, { static_cast(added_message), static_cast(added_message) }, false, { "\\Deleted" }, false))->await()).release_value(); outln("[STORE] Store response: {}", response.response_text()); - response = move(TRY(client->send_simple_command(IMAP::CommandType::Expunge)->await()).release_value().get()); + response = move(TRY(TRY(client->send_simple_command(IMAP::CommandType::Expunge))->await()).release_value().get()); outln("[EXPUNGE] Number of expunged entries: {}", response.data().expunged().size()); if (idle_supported) { - VERIFY(TRY(client->idle()->await()).has_value()); + VERIFY(TRY(TRY(client->idle())->await()).has_value()); sleep(3); - response = TRY(client->finish_idle()->await()).release_value(); + response = TRY(TRY(client->finish_idle())->await()).release_value(); outln("[IDLE] Idle response: {}", response.response_text()); } else { outln("[IDLE] Skipped. No IDLE support."); } - response = move(TRY(client->send_simple_command(IMAP::CommandType::Logout)->await()).release_value().get()); + response = move(TRY(TRY(client->send_simple_command(IMAP::CommandType::Logout))->await()).release_value().get()); outln("[LOGOUT] Bye: {}", response.data().bye_message().value()); client->close();