Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"message" event doesn't fire for some types of messages #51

Closed
vu0tran opened this issue Jul 14, 2015 · 8 comments
Closed

"message" event doesn't fire for some types of messages #51

vu0tran opened this issue Jul 14, 2015 · 8 comments

Comments

@vu0tran
Copy link

vu0tran commented Jul 14, 2015

When I test switchboard by sending a test email from Mandrill, the "message" event never fires for some reason.

It works for all other types of emails... except emails from Mandrill. Any ideas? Here's how I'm sending them via the mandrill-api gem:

  mandrill = Mandrill::API.new 'API_KEY_HERE'
  message = {"auto_text"=>nil,
   "text"=>params['body'],
   "to"=>
      [{"type"=>"to",
          "email"=>'TO_EMAIL_HERE',
          "name"=>""}],
   "from_name"=>"Foo",
   "from_email"=>"[email protected]",
   "subject"=>'TEST SUBJECT',
   "headers"=>{"Reply-To"=>"[email protected]"},
   "important"=>false}
  async = false
  result = mandrill.messages.send message, async

  puts "sent"
@jtmoulia
Copy link
Contributor

Hello -- strange issue.

The message definitely being delivered to the email account's mailbox being monitored by switchboard? If it's, say, being caught by a spam filter switchboard wouldn't publish a notification.

Otherwise, looks like I'll get to check out mandrill :)

@vu0tran
Copy link
Author

vu0tran commented Jul 14, 2015

It's not the spam filter unfortunately. I'm 100% sure it's being monitored as any email I immediately send afterwards with a regular Gmail account works just fine.

I can point the script at your email account and help test if you'd like. We can take it offline if you'd like to message me at [email protected].

@jtmoulia
Copy link
Contributor

Sounds good -- I emailed you, and will be trying to replicate on my side.

@jtmoulia
Copy link
Contributor

jtmoulia commented Aug 7, 2015

There's been progress on this issue over email (thanks @Ostera!), but now that we're hammering out the details of the fix I thought we should move it back to Github. Here's the thread for background:

Leandro Ostera writes:

I think we can standardize it to

{address, [ {name, "leandro ostera"}, {mailbox, "leandro"}, {host, "
gmail.com"} ] }

And that'd make it pretty easy to deal with either an empty mailbox, or an
empty host (like the case we're dealing with). Plus the actual email is a
concatenation away ( <<mailbox/binary, $@, domain/binary>> ).

Having only {email, nil} doesn't really tell us if the mailbox or the host
is nil, but that one of them is missing.

Does that make sense? I'm working on a monkey-patch for it in the meantime

On Tue, Aug 4, 2015 at 12:14 AM, Thomas Moulia [email protected]
wrote:

I was wondering in the last email whether this issue was due to the
RFC2822 group
syntax, but nope:

([email protected])4> 18:13:55.899 [error] gen_server <0.235.0>

terminated with reason: no function clause matching
imap:clean_addresses([[{string,<<"Calendar <
[email protected]>">>},nil,{string,<<"Google">>},nil]],
[]) line 877

The single address is simply NIL. It does have the full address in the
address name field, e.g. "Calendar [email protected]",
however I don't think that can be guaranteed, like in Leandro's example.

My vote is that we need to handle this case, however this means that the
'address' data structures no longer necessarily have an email property
(or that the value might be NIL).

More specifically, at the moment when the host field is not nil we
translate:

("leandro ostera" NIL "leandro" "gmail.com")

to: {address, [{name, "leandro ostera"}, {email, "[email protected]}]}

For the cases where the host is NIL:

("leandro ostera" NIL "leandro" NIL)

might become: {address, [{name, "leandro ostera"}, {mailbox, "leandro"}]},
or: {address, [{name, "leandro ostera"}, {email, nil}, {mailbox,
"leandro"}]}

Preferences? Thoughts?

Cheers,
-Thomas

Thomas Moulia writes:

Awesome, Leandro -- you're way ahead of me.

Some poking around shows that The IMAP protocol allows for the fourth
param, the host name field, to be NIL given the RFC2822 group syntax.

From RFC3501:

[RFC-2822] group syntax is indicated by a special form of address
structure in which the host name field is NIL. If the mailbox name
field is also NIL, this is an end of group marker (semi-colon in RFC
822 syntax). If the mailbox name field is non-NIL, this is a start of
group marker, and the mailbox name field holds the group name phrase.

I'll have to check out exactly what is being returned, but this
indicates that

("leandro ostera" NIL "leandro" NIL)

is the "start of group marker" for a group named "leandro". But, then
there should also be a subsequent "end of group marker" address (or a
semi-colon?) similar to:

("some name?" NIL NIL NIL)

clean_address/2 should definitely handle this case, though I'm not
sure if this group address thing is what's actually happening.

More generally, I'd prefer switchboard to be pragmatic rather than
correct. If major companies like Mandrill and Google are sending
non-group addresses which are simply the following, switchboard should
handle it.

(("leandro ostera" NIL "leandro" NIL))

Is this the case?

Thanks!
-Thomas

Leandro Ostera writes:

Hey guys,

I narrowed it down to neither Mandrill by default, nor Google at all,
sending complete ENVELOPE email addresses. That is, the IMAP protocol
defines the ENVELOPE's email addresses as follow:

(("Name on Email Account" NIL "emailaddress" "domain.com"))

What I saw coming from Mandrill had no fourth param, but instead NIL.
Something like this:

(("leandro ostera" NIL "leandro" NIL))

That was fixed after I figured out 2 things:

  1. Neither DKIM nor SPF where verified, and
  2. Setting the ReturnPath via Mandrill's API (

https://mandrill.zendesk.com/hc/en-us/articles/205582727-Can-I-customize-the-Return-Path-bounce-address-used-for-my-emails-
)

Now although those emails are being processed alrighty, the ones that
still crash are Google Calendar Invites (and who knows how many others).

The reason for the crash is that in the file imap.erl:878, the function
clean_addresses/2, has the following signature:

clean_addresses([[RawName, _, {string, MailBox}, {string, Host}] |
Rest],

Acc)

And when being called with the above example, thus failing to match
the {string,
Host} and crashing the process – of course after this, no data is being
sent to the workers, and thus no push notification is being sent to the
devices.

I think the question here is whether the protocol specifies this
rigidity
in the email descriptors for the ENVELOPE, and whether we can bypass it
sensibly in order to provide as much reliability as possible without
compromising the amount of external services that do not work with the
push
system

– in other words, monkey patching clean_addresses to deal with missing
Host
strings would allow this emails to go through, but it'd also open a lot
of
other cases that I'm unaware of at the moment. I'd evaluate this over
the
week.

Along these lines, I'll get back in touch if I find anything else!

Cheers,
Leandro

@leostera
Copy link
Contributor

leostera commented Aug 7, 2015

I have the following workaround running seemingly fine:

clean_addresses([[RawName, _, {string, MailBox}, Host] | Rest], Acc) ->
    Address = case Host of 
                {string, Domain} -> [{email, <<MailBox/binary, $@, Domain/binary>>}];
                nil -> [{email, nil}]
              end,
    clean_addresses(Rest,
                    [{address, case RawName of
                                   nil -> [{name, <<"">>} | Address];
                                   {string, Name} -> [{name, Name} | Address]
                               end} |
                     Acc]).

Where I'm deferring the Host pattern matching to a case within the function body. Seems to be working okay, but ideally we'd have something more robust as it's hard for me right now to see the side-effects of this.

@leostera
Copy link
Contributor

I'll submit a pull request that handles the email: joe@ as per jmapio/jmap#13 either today or tomorrow.

@leostera
Copy link
Contributor

@jtmoulia implementation wise, a build_address private method that handles the Host being empty and just returns the built string should suffice IMO, but I'm open to suggestions.

@jtmoulia
Copy link
Contributor

Sounds awesome, @Ostera. Another function, build_address make sense. Alternatively, you could do something similar to the way you modified clean_addresses above. Whichever makes the most sense to you.

leostera added a commit to leostera/switchboard that referenced this issue Aug 10, 2015
jtmoulia added a commit that referenced this issue Aug 12, 2015
Fixes handling of no Host in an email address description

Resolves #51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants