From 6c8f192148ef76054a267651718ec32f21eeb208 Mon Sep 17 00:00:00 2001 From: Michael Gratton Date: Sat, 9 Mar 2019 20:06:45 +1100 Subject: [PATCH] Attempt to de-mangle From names from Mailman, GitLab, etc Some software like the above will mangle From mailbox names by appending "via Some Service" to the From mailbox name. This messes up generating default avatars for the actual people sending these messages, so attempt to de-mangle the names. This involves moving primary originator determination from the engine to the client, since it's now a policy thing. Add unit tests. --- .../conversation-email.vala | 16 ++- .../conversation-message.vala | 4 +- src/client/notification/libnotify.vala | 3 +- src/client/util/util-email.vala | 59 +++++++++ src/engine/api/geary-email.vala | 19 --- src/engine/rfc822/rfc822-message.vala | 19 --- test/client/util/util-email-test.vala | 115 ++++++++++++++++++ test/meson.build | 2 + test/test-client.vala | 1 + 9 files changed, 192 insertions(+), 46 deletions(-) create mode 100644 test/client/util/util-email-test.vala diff --git a/src/client/conversation-viewer/conversation-email.vala b/src/client/conversation-viewer/conversation-email.vala index 3a94b2f1..ea57e20e 100644 --- a/src/client/conversation-viewer/conversation-email.vala +++ b/src/client/conversation-viewer/conversation-email.vala @@ -289,6 +289,9 @@ public class ConversationEmail : Gtk.Box, Geary.BaseInterface { /** Determines if the email is a draft message. */ public bool is_draft { get; private set; } + /** The email's primary originator, if any. */ + public Geary.RFC822.MailboxAddress? primary_originator { get; private set; } + /** The view displaying the email's primary message headers and body. */ public ConversationMessage primary_message { get; private set; } @@ -444,6 +447,7 @@ public class ConversationEmail : Gtk.Box, Geary.BaseInterface { base_ref(); this.email = email; this.is_draft = is_draft; + this.primary_originator = Util.Email.get_primary_originator(email); this.email_store = email_store; this.contact_store = email_store.account.get_contact_store(); this.avatar_store = avatar_store; @@ -507,9 +511,11 @@ public class ConversationEmail : Gtk.Box, Geary.BaseInterface { // Construct the view for the primary message, hook into it bool load_images = email.load_remote_images().is_certain(); - Geary.Contact contact = this.contact_store.get_by_rfc822( - email.get_primary_originator() - ); + + Geary.Contact? contact = null; + if (this.primary_originator != null) { + contact = this.contact_store.get_by_rfc822(this.primary_originator); + } if (contact != null) { load_images |= contact.always_load_remote_images(); } @@ -577,7 +583,7 @@ public class ConversationEmail : Gtk.Box, Geary.BaseInterface { } catch (IOError.CANCELLED err) { // okay } catch (Error err) { - Geary.RFC822.MailboxAddress? from = this.email.get_primary_originator(); + Geary.RFC822.MailboxAddress? from = this.primary_originator; debug("Avatar load failed for \"%s\": %s", from != null ? from.to_string() : "", err.message); } @@ -1038,7 +1044,7 @@ public class ConversationEmail : Gtk.Box, Geary.BaseInterface { private void on_remember_remote_images(ConversationMessage view) { - Geary.RFC822.MailboxAddress? sender = this.email.get_primary_originator(); + Geary.RFC822.MailboxAddress? sender = this.primary_originator; if (sender != null) { Geary.Contact? contact = this.contact_store.get_by_rfc822(sender); if (contact != null) { diff --git a/src/client/conversation-viewer/conversation-message.vala b/src/client/conversation-viewer/conversation-message.vala index 17614426..e8cd4f27 100644 --- a/src/client/conversation-viewer/conversation-message.vala +++ b/src/client/conversation-viewer/conversation-message.vala @@ -289,7 +289,7 @@ public class ConversationMessage : Gtk.Grid, Geary.BaseInterface { bool load_remote_images, Configuration config) { this( - email.get_primary_originator(), + Util.Email.get_primary_originator(email), email.from, email.reply_to, email.sender, @@ -315,7 +315,7 @@ public class ConversationMessage : Gtk.Grid, Geary.BaseInterface { bool load_remote_images, Configuration config) { this( - message.get_primary_originator(), + Util.Email.get_primary_originator(message), message.from, message.reply_to, message.sender, diff --git a/src/client/notification/libnotify.vala b/src/client/notification/libnotify.vala index d82a5a9c..11bf793f 100644 --- a/src/client/notification/libnotify.vala +++ b/src/client/notification/libnotify.vala @@ -95,7 +95,8 @@ public class Libnotify : Geary.BaseObject { return; // possible to receive email with no originator - Geary.RFC822.MailboxAddress? primary = email.get_primary_originator(); + Geary.RFC822.MailboxAddress? primary = + Util.Email.get_primary_originator(email); if (primary == null) { notify_new_mail(folder, 1); diff --git a/src/client/util/util-email.vala b/src/client/util/util-email.vala index 05a82828..55bda5a1 100644 --- a/src/client/util/util-email.vala +++ b/src/client/util/util-email.vala @@ -38,6 +38,65 @@ namespace Util.Email { return !Geary.String.is_empty(cleaned) ? cleaned : _("(no subject)"); } + /** + * Returns a mailbox for the primary originator of an email. + * + * RFC 822 allows multiple and absent From header values, and + * software such as Mailman and GitLab will mangle the names in + * From mailboxes. This provides a canonical means to obtain a + * mailbox (that is, name and email address) for the first + * originator, and with the mailbox's name having been fixed up + * where possible. + * + * The first From mailbox is used and de-mangled if found, if not + * the Sender mailbox is used if present, else the first Reply-To + * mailbox is used. + */ + public Geary.RFC822.MailboxAddress? + get_primary_originator(Geary.EmailHeaderSet email) { + Geary.RFC822.MailboxAddress? primary = null; + if (email.from != null && email.from.size > 0) { + // We have a From address, so attempt to de-mangle it + Geary.RFC822.MailboxAddresses? from = email.from; + + string from_name = ""; + if (from != null && from.size > 0) { + primary = from[0]; + from_name = primary.name ?? ""; + } + + Geary.RFC822.MailboxAddresses? reply_to = email.reply_to; + Geary.RFC822.MailboxAddress? primary_reply_to = null; + string reply_to_name = ""; + if (reply_to != null && reply_to.size > 0) { + primary_reply_to = reply_to[0]; + reply_to_name = primary_reply_to.name ?? ""; + } + + // Spaces are important + const string VIA = " via "; + + if (reply_to_name != "" && from_name.has_prefix(reply_to_name)) { + // Mailman sometimes sends the true originator as the + // Reply-To for the email + primary = primary_reply_to; + } else if (VIA in from_name) { + // Mailman, GitLib, Discourse and others send the + // originator's name prefixing something starting with + // "via". + primary = new Geary.RFC822.MailboxAddress( + from_name.split(VIA, 2)[0], primary.address + ); + } + } else if (email.sender != null) { + primary = email.sender; + } else if (email.reply_to != null && email.reply_to.size > 0) { + primary = email.reply_to[0]; + } + + return primary; + } + /** * Returns a shortened recipient list suitable for display. * diff --git a/src/engine/api/geary-email.vala b/src/engine/api/geary-email.vala index e22d5762..2b4d7ec4 100644 --- a/src/engine/api/geary-email.vala +++ b/src/engine/api/geary-email.vala @@ -530,25 +530,6 @@ public class Geary.Email : BaseObject, EmailHeaderSet { return (preview != null) ? preview.buffer.to_string() : ""; } - /** - * Returns the primary originator of an email, which is defined as the first mailbox address - * in From:, Sender:, or Reply-To:, in that order, depending on availability. - * - * Returns null if no originators are present. - */ - public RFC822.MailboxAddress? get_primary_originator() { - if (from != null && from.size > 0) - return from[0]; - - if (sender != null) - return sender; - - if (reply_to != null && reply_to.size > 0) - return reply_to[0]; - - return null; - } - public string to_string() { return "[%s] ".printf(id.to_string()); } diff --git a/src/engine/rfc822/rfc822-message.vala b/src/engine/rfc822/rfc822-message.vala index 9bbfb5a0..5f70b854 100644 --- a/src/engine/rfc822/rfc822-message.vala +++ b/src/engine/rfc822/rfc822-message.vala @@ -441,25 +441,6 @@ public class Geary.RFC822.Message : BaseObject, EmailHeaderSet { : ""; } - /** - * Returns the primary originator of an email, which is defined as the first mailbox address - * in From:, Sender:, or Reply-To:, in that order, depending on availability. - * - * Returns null if no originators are present. - */ - public RFC822.MailboxAddress? get_primary_originator() { - if (from != null && from.size > 0) - return from[0]; - - if (sender != null) - return sender; - - if (reply_to != null && reply_to.size > 0) - return reply_to[0]; - - return null; - } - public Gee.List? get_recipients() { Gee.List addrs = new Gee.ArrayList(); diff --git a/test/client/util/util-email-test.vala b/test/client/util/util-email-test.vala new file mode 100644 index 00000000..3630276c --- /dev/null +++ b/test/client/util/util-email-test.vala @@ -0,0 +1,115 @@ +/* + * Copyright 2019 Michael Gratton + * + * This software is licensed under the GNU Lesser General Public License + * (version 2.1 or later). See the COPYING file in this distribution. + */ + +public class Util.Email.Test : TestCase { + + public Test() { + base("UtilEmailTest"); + add_test("null_originator", null_originator); + add_test("from_originator", from_originator); + add_test("sender_originator", sender_originator); + add_test("reply_to_originator", reply_to_originator); + add_test("reply_to_via_originator", reply_to_via_originator); + add_test("plain_via_originator", plain_via_originator); + } + + public void null_originator() throws GLib.Error { + Geary.RFC822.MailboxAddress? originator = get_primary_originator( + new_email(null, null, null) + ); + + assert_null(originator); + } + + public void from_originator() throws GLib.Error { + Geary.RFC822.MailboxAddress? originator = get_primary_originator( + new_email( + new Geary.RFC822.MailboxAddress("from", "from@example.com"), + new Geary.RFC822.MailboxAddress("sender", "sender@example.com"), + new Geary.RFC822.MailboxAddress("reply-to", "reply-to@example.com") + ) + ); + + assert_non_null(originator); + assert_string("from", originator.name); + assert_string("from@example.com", originator.address); + } + + public void sender_originator() throws GLib.Error { + Geary.RFC822.MailboxAddress? originator = get_primary_originator( + new_email( + null, + new Geary.RFC822.MailboxAddress("sender", "sender@example.com"), + new Geary.RFC822.MailboxAddress("reply-to", "reply-to@example.com") + ) + ); + + assert_non_null(originator); + assert_string("sender", originator.name); + assert_string("sender@example.com", originator.address); + } + + public void reply_to_originator() throws GLib.Error { + Geary.RFC822.MailboxAddress? originator = get_primary_originator( + new_email( + null, + null, + new Geary.RFC822.MailboxAddress("reply-to", "reply-to@example.com") + ) + ); + + assert_non_null(originator); + assert_string("reply-to", originator.name); + assert_string("reply-to@example.com", originator.address); + } + + public void reply_to_via_originator() throws GLib.Error { + Geary.RFC822.MailboxAddress? originator = get_primary_originator( + new_email( + new Geary.RFC822.MailboxAddress("test via bot", "bot@example.com"), + null, + new Geary.RFC822.MailboxAddress("test", "test@example.com") + ) + ); + + assert_non_null(originator); + assert_string("test", originator.name); + assert_string("test@example.com", originator.address); + } + + public void plain_via_originator() throws GLib.Error { + Geary.RFC822.MailboxAddress? originator = get_primary_originator( + new_email( + new Geary.RFC822.MailboxAddress("test via bot", "bot@example.com"), + null, + null + ) + ); + + assert_non_null(originator); + assert_string("test", originator.name); + assert_string("bot@example.com", originator.address); + } + + private Geary.Email new_email(Geary.RFC822.MailboxAddress? from, + Geary.RFC822.MailboxAddress? sender, + Geary.RFC822.MailboxAddress? reply_to) + throws GLib.Error { + Geary.Email email = new Geary.Email(new Geary.MockEmailIdentifer(1)); + email.set_originators( + from != null + ? new Geary.RFC822.MailboxAddresses(Geary.Collection.single(from)) + : null, + sender, + reply_to != null + ? new Geary.RFC822.MailboxAddresses(Geary.Collection.single(reply_to)) + : null + ); + return email; + } + +} diff --git a/test/meson.build b/test/meson.build index 6054796a..ab3ea78c 100644 --- a/test/meson.build +++ b/test/meson.build @@ -68,6 +68,7 @@ geary_test_client_sources = [ # and the engine test sute needs to depend # geary-engine_internal.vapi, which leads to duplicate symbols when # linking + 'engine/api/geary-email-identifier-mock.vala', 'engine/api/geary-credentials-mediator-mock.vala', 'client/accounts/accounts-manager-test.vala', @@ -76,6 +77,7 @@ geary_test_client_sources = [ 'client/components/client-web-view-test-case.vala', 'client/composer/composer-web-view-test.vala', 'client/util/util-avatar-test.vala', + 'client/util/util-email-test.vala', 'js/client-page-state-test.vala', 'js/composer-page-state-test.vala', diff --git a/test/test-client.vala b/test/test-client.vala index 3386d183..1852ba6e 100644 --- a/test/test-client.vala +++ b/test/test-client.vala @@ -44,6 +44,7 @@ int main(string[] args) { client.add_suite(new ComposerWebViewTest().get_suite()); client.add_suite(new ConfigurationTest().get_suite()); client.add_suite(new Util.Avatar.Test().get_suite()); + client.add_suite(new Util.Email.Test().get_suite()); TestSuite js = new TestSuite("js");