launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01386
[Merge] lp:~gary/launchpad/bug650343 into lp:launchpad/devel
Gary Poster has proposed merging lp:~gary/launchpad/bug650343 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
#650343 Add X-Launchpad-Original-To to recipient lists
https://bugs.launchpad.net/bugs/650343
See bug 65043 for the motivation. This is primarily just reverting the change done described in that bug, because the constraint/problem in the data center email configuration has been resolved.
I kept a spelling correction from the change. I also changed things per what lint described (e.g., converted to ReST), except for the singleton tuple complaints (e.g., in "log.warning('DKIM error: %r' % (e,))" the linter wants to see "(e, )").
./lib/canonical/launchpad/mail/incoming.py
126: E231 missing whitespace after ','
130: E231 missing whitespace after ','
133: E231 missing whitespace after ','
134: E231 missing whitespace after ','
142: E231 missing whitespace after ','
153: E231 missing whitespace after ','
--
https://code.launchpad.net/~gary/launchpad/bug650343/+merge/37633
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gary/launchpad/bug650343 into lp:launchpad/devel.
=== modified file 'lib/canonical/launchpad/doc/incomingmail.txt'
--- lib/canonical/launchpad/doc/incomingmail.txt 2010-04-26 12:20:32 +0000
+++ lib/canonical/launchpad/doc/incomingmail.txt 2010-10-05 16:22:45 +0000
@@ -1,4 +1,5 @@
-= Incoming Mail =
+Incoming Mail
+=============
When an email is sent to Launchpad we need to handle it somehow. This
is done by handleEmails:
@@ -14,7 +15,9 @@
* Deletes the message from the mail box
-== Mail Handlers ==
+-------------
+Mail Handlers
+-------------
A mail handler is a utility which knows how to handle mail sent to a
specific domain. It is registered as a named utility providing
@@ -82,6 +85,10 @@
handleMail, so that every mail gets handled by the correct handler.
>>> handleMail(zopeless_transaction)
+ WARNING:canonical.launchpad.mail:No X-Original-To header was present ...
+ WARNING:canonical.launchpad.mail:No X-Original-To header was present ...
+ WARNING:canonical.launchpad.mail:No X-Original-To header was present ...
+ WARNING:canonical.launchpad.mail:No X-Original-To header was present ...
Now we can see that each handler handled the emails sent to its domain:
@@ -90,8 +97,9 @@
>>> set(bar_handler.handledMails) ^ set(msgids['bar.com'])
set([])
-
-== Unhandled Mail ==
+--------------
+Unhandled Mail
+--------------
So, what happened to the message that got sent to baz.com? Since there
wasn't a handler registered for that domain, an OOPS was recorded with
@@ -118,8 +126,9 @@
>>> stub.test_emails = []
-
-== Mail from Persons not registered in Launchpad ==
+---------------------------------------------
+Mail from Persons not registered in Launchpad
+---------------------------------------------
If a Person who isn't registered in Launchpad sends an email, we'll
most of the time reject the email:
@@ -140,7 +149,7 @@
So if we send the mail to bar.com, bar_handler will handle the mail:
- >>> moin_change.replace_header('To', '123@xxxxxxx')
+ >>> moin_change.replace_header('X-Original-To', '123@xxxxxxx')
>>> msgid = "<%s>" % sendmail(moin_change)
>>> handleMail(zopeless_transaction)
>>> msgid in bar_handler.handledMails
@@ -148,8 +157,9 @@
>>> stub.test_emails = []
-
-== Mail from Persons with with an inactive Launchpad account ==
+---------------------------------------------------------
+Mail from Persons with with an inactive Launchpad account
+---------------------------------------------------------
If a Person who's account is inactive sends an email, it will be
silently rejected.
@@ -173,15 +183,31 @@
>>> msgid not in foo_handler.handledMails
True
-== X-Original-To ==
-
-Ideally, we would use X-Original-To, as this reflects the actual address
-that the mail was sent to. Unfortunately, due to mail configuration issues,
-X-Original-To is very wrong, so we must rely on To and Cc, even though these
-may be inaccurate.
-
-
-== OOPSes processing incoming mail ==
+-------------
+X-Original-To
+-------------
+
+If available, the X-Original-To header is used to determine to which
+address the email was sent to:
+
+ >>> msg = read_test_message('signed_detached.txt')
+ >>> msg.replace_header('To', '123@xxxxxxx')
+ >>> msg['CC'] = '123@xxxxxxx'
+ >>> msg['X-Original-To'] = '123@xxxxxxx'
+ >>> msgid = '<%s>' % sendmail (msg, ['123@xxxxxxx'])
+ >>> handleMail(zopeless_transaction)
+ >>> msgid in bar_handler.handledMails
+ True
+
+Only the address in X-Original-To header will be used. The addresses in
+the To and CC headers will be ignored:
+
+ >>> msgid in foo_handler.handledMails
+ False
+
+-------------------------------
+OOPSes processing incoming mail
+-------------------------------
If an unhandled exception occurs when we try to process an email from
a user, we record an OOPS with the exception and send it to the user.
@@ -283,8 +309,9 @@
>>> stub.test_emails = []
-
-== DB exceptions ==
+-------------
+DB exceptions
+-------------
If something goes wrongs in the handler, a DB exception can be raised,
leaving the database in a bad state. If that happens a traceback should
@@ -328,6 +355,7 @@
...
DataError: division by zero
<BLANKLINE>
+ WARNING...
The second mail we sent got handled despite the exception:
@@ -340,8 +368,9 @@
>>> len(stub.test_emails)
1
-
-== Librarian not running ==
+---------------------
+Librarian not running
+---------------------
If for some reason the Librarian isn't up and running, we shouldn't
lose any emails. All that should happen is that an error should get
@@ -368,8 +397,9 @@
>>> LibrarianLayer.reveal()
>>> stub.test_emails = []
-
-== Handling bounces ==
+----------------
+Handling bounces
+----------------
Some broken mailers might not respect the Errors-To and Return-Path
headers, send error messages back to the address, from which the email
@@ -427,7 +457,7 @@
0
-== Doctest cleanup ==
+.. Doctest cleanup
>>> config_data = config.pop('bugmail_error_from_address')
>>> mail_handlers.add('foo.com', None)
=== modified file 'lib/canonical/launchpad/mail/incoming.py'
--- lib/canonical/launchpad/mail/incoming.py 2010-08-20 20:31:18 +0000
+++ lib/canonical/launchpad/mail/incoming.py 2010-10-05 16:22:45 +0000
@@ -149,7 +149,7 @@
% (signing_domain, from_domain))
return False
if not _isDkimDomainTrusted(signing_domain):
- log.warning("valid DKIM signature from untrusted domain %s"
+ log.warning("valid DKIM signature from untrusted domain %s"
% (signing_domain,))
return False
return True
@@ -338,8 +338,7 @@
if mail['Return-Path'] == '<>':
_handle_error(
"Message had an empty Return-Path.",
- file_alias_url, notify=False
- )
+ file_alias_url, notify=False)
continue
if mail.get_content_type() == 'multipart/report':
# Mails with a content type of multipart/report are
@@ -351,8 +350,7 @@
if 'precedence' in mail:
_handle_error(
"Got a message with a precedence header.",
- file_alias_url, notify=False
- )
+ file_alias_url, notify=False)
continue
try:
@@ -367,13 +365,19 @@
continue
# Extract the domain the mail was sent to. Mails sent to
- # Launchpad should have an X-Original-To header, but
- # it has an incorrect address.
- # Process all addresses found as a fall back.
- cc = mail.get_all('cc') or []
- to = mail.get_all('to') or []
- names_addresses = getaddresses(to + cc)
- addresses = [addr for name, addr in names_addresses]
+ # Launchpad should have an X-Original-To header.
+ if 'X-Original-To' in mail:
+ addresses = [mail['X-Original-To']]
+ else:
+ log = logging.getLogger('canonical.launchpad.mail')
+ log.warn(
+ "No X-Original-To header was present in email: %s" %
+ file_alias_url)
+ # Process all addresses found as a fall back.
+ cc = mail.get_all('cc') or []
+ to = mail.get_all('to') or []
+ names_addresses = getaddresses(to + cc)
+ addresses = [addr for name, addr in names_addresses]
try:
do_paranoid_envelope_to_validation(addresses)
@@ -400,8 +404,7 @@
if principal is None and not handler.allow_unknown_users:
_handle_error(
'Unknown user: %s ' % mail['From'],
- file_alias_url, notify=False
- )
+ file_alias_url, notify=False)
continue
handled = handler.process(mail, email_addr, file_alias)
=== modified file 'lib/lp/blueprints/doc/spec-mail-exploder.txt'
--- lib/lp/blueprints/doc/spec-mail-exploder.txt 2010-07-28 16:56:05 +0000
+++ lib/lp/blueprints/doc/spec-mail-exploder.txt 2010-10-05 16:22:45 +0000
@@ -264,7 +264,7 @@
True
>>> moin_change = read_test_message('moin-change.txt')
- >>> moin_change['To'] += ", notifications@%s" % (
+ >>> moin_change['X-Original-To'] = "notifications@%s" % (
... config.launchpad.specs_domain)
>>> moin_change['Sender'] = 'webmaster@xxxxxxxxxx'
Follow ups
