Commit: 848e2f4f15eafd9a2826cbac86fe9bfd539db557 - allura (git)

allura

Commit MetaInfo

• Revision: 848e2f4f15eafd9a2826cbac86fe9bfd539db557 (tree)
• Zeit: 2012-05-04 02:22:14
• Autor: Dave Brondsema <dbrondsema@geek...>
• Commiter: Yaroslav Luzin

Log Message

[#4103] fix several encoding issues with mail sending

* Deeper mail tests
* Don't encode headers in parts - don't even encode it at all. Just
ensure it is unicode, and let email.header.Header do the right thing
* email body attempted as ascii instead of latin1, before going to utf-8

Ändern Zusammenfassung

• modified: Allura/allura/lib/mail_util.py
• modified: Allura/allura/tasks/mail_tasks.py
• modified: Allura/allura/tests/functional/test_auth.py
• modified: Allura/allura/tests/model/test_notification.py
• modified: Allura/allura/tests/test_mail_util.py
• modified: Allura/allura/tests/test_tasks.py
• modified: Allura/allura/websetup/bootstrap.py

Diff

--- a/Allura/allura/lib/mail_util.py
+++ b/Allura/allura/lib/mail_util.py
@@ -23,16 +23,39 @@ config = ConfigProxy(
     return_path='forgemail.return_path')
 EMAIL_VALIDATOR=fev.Email(not_empty=True)
 
-def Header(text, charset):
-    '''Helper to make sure we don't over-encode headers
-
-    (gmail barfs with encoded email addresses.)'''
+def Header(text, *more_text):
+    '''Helper to make sure we encode headers properly'''
     if isinstance(text, header.Header):
         return text
-    hdr = header.Header('', charset)
-    for word in text.split(' '):
-        hdr.append(word)
-    return hdr
+    # email.header.Header handles str vs unicode differently
+    # see http://docs.python.org/library/email.header.html#email.header.Header.append
+    if type(text) != unicode:
+        raise TypeError('This must be unicode: %r' % text)
+    head = header.Header(text)
+    for m in more_text:
+        if type(m) != unicode:
+            raise TypeError('This must be unicode: %r' % text)
+        head.append(m)
+    return head
+
+def AddrHeader(fromaddr):
+    '''Accepts any of:
+        Header() instance
+        [e-mail]
+        "Foo Bar" <[e-mail]>
+    '''
+    if isinstance(fromaddr, basestring) and ' <' in fromaddr:
+        name, addr = fromaddr.rsplit(' <', 1)
+        addr = '<' + addr # restore the char we just split off
+        addrheader = Header(name, addr)
+        if str(addrheader).startswith('=?'): # encoding escape chars
+            # then quoting the name is no longer necessary
+            name = name.strip('"')
+            addrheader = Header(name, addr)
+    else:
+        addrheader = Header(fromaddr)
+    return addrheader
+
 
 def parse_address(addr):
     userpart, domain = addr.split('@')
@@ -103,7 +126,7 @@ def identify_sender(peer, email_address, headers, msg):
 
 def encode_email_part(content, content_type):
     try:
-        return MIMEText(content.encode('iso-8859-1'), content_type, 'iso-8859-1')
+        return MIMEText(content.encode('ascii'), content_type, 'ascii')
     except:
         return MIMEText(content.encode('utf-8'), content_type, 'utf-8')
 
@@ -142,21 +165,19 @@ class SMTPClient(object):
     def __init__(self):
         self._client = None
 
-    def sendmail(self, addrs, addrfrom, reply_to, subject, message_id, in_reply_to, message):
+    def sendmail(self, addrs, fromaddr, reply_to, subject, message_id, in_reply_to, message):
         if not addrs: return
-        charset = message.get_charset()
-        if charset is None:
-            charset = 'iso-8859-1'
-        message['To'] = Header(reply_to, charset)
-        message['From'] = Header(addrfrom, charset)
-        message['Reply-To'] = Header(reply_to, charset)
-        message['Subject'] = Header(subject, charset)
-        message['Message-ID'] = Header('<' + message_id + '>', charset)
+        # We send one message with multiple envelope recipients, so use a generic To: addr
+        # It might be nice to refactor to send one message per recipient, and use the actual To: addr
+        message['To'] = Header(reply_to)
+        message['From'] = AddrHeader(fromaddr)
+        message['Reply-To'] = Header(reply_to)
+        message['Subject'] = Header(subject)
+        message['Message-ID'] = Header('<' + message_id + u'>')
         if in_reply_to:
-            if isinstance(in_reply_to, basestring):
-                in_reply_to = [ in_reply_to ]
-            in_reply_to = ','.join(('<' + irt + '>') for irt in in_reply_to)
-            message['In-Reply-To'] = Header(in_reply_to, charset)
+            if not isinstance(in_reply_to, basestring):
+                raise TypeError('Only strings are supported now, not lists')
+            message['In-Reply-To'] = Header(u'<%s>' % in_reply_to)
         content = message.as_string()
         smtp_addrs = map(_parse_smtp_addr, addrs)
         smtp_addrs = [ a for a in smtp_addrs if isvalid(a) ]

--- a/Allura/allura/tasks/mail_tasks.py
+++ b/Allura/allura/tasks/mail_tasks.py
@@ -56,14 +56,8 @@ def route_email(
             log.exception('Error routing mail to %s', addr)
 
 @task
-def sendmail(
-    fromaddr,
-    destinations,
-    text,
-    reply_to,
-    subject,
-    message_id,
-    in_reply_to=None):
+def sendmail(fromaddr, destinations, text, reply_to, subject,
+             message_id, in_reply_to=None):
     from allura import model as M
     addrs_plain = []
     addrs_html = []

--- a/Allura/allura/tests/functional/test_auth.py
+++ b/Allura/allura/tests/functional/test_auth.py
@@ -16,7 +16,8 @@ class TestAuth(TestController):
     def test_login(self):
         result = self.app.get('/auth/')
         r = self.app.post('/auth/send_verification_link', params=dict(a='[e-mail]'))
-        r = self.app.post('/auth/send_verification_link', params=dict(a='[e-mail]'))
+        email = M.User.query.get(username='test-admin').email_addresses[0]
+        r = self.app.post('/auth/send_verification_link', params=dict(a=email))
         ThreadLocalORMSession.flush_all()
         r = self.app.get('/auth/verify_addr', params=dict(a='foo'))
         assert json.loads(self.webflash(r))['status'] == 'error', self.webflash(r)

--- a/Allura/allura/tests/model/test_notification.py
+++ b/Allura/allura/tests/model/test_notification.py
@@ -2,7 +2,7 @@ import unittest
 from datetime import timedelta
 
 from pylons import g, c
-
+from nose.tools import assert_equal
 from ming.orm import ThreadLocalORMSession
 
 from alluratest.controller import setup_basic_test, setup_global_objects, REGISTRY
@@ -103,7 +103,7 @@ class TestPostNotifications(unittest.TestCase):
         ThreadLocalORMSession.flush_all()
         M.MonQTask.run_ready()
         ThreadLocalORMSession.flush_all()
-        assert M.Notification.query.get()['from_address'].startswith('"Test Admin" <Beta')
+        assert_equal(M.Notification.query.get()['from_address'], '"Test Admin" <[e-mail]>')
         assert M.Mailbox.query.find().count()==1
         mbox = M.Mailbox.query.get()
         assert len(mbox.queue) == 1
@@ -232,4 +232,3 @@ def _clear_subscriptions():
 
 def _clear_notifications():
         M.Notification.query.remove({})
-

--- a/Allura/allura/tests/test_mail_util.py
+++ b/Allura/allura/tests/test_mail_util.py
@@ -2,6 +2,7 @@
 import unittest
 from email.MIMEMultipart import MIMEMultipart
 from email.MIMEText import MIMEText
+from email import header
 
 from nose.tools import raises, assert_equal
 from ming.orm import ThreadLocalORMSession
@@ -9,7 +10,7 @@ from ming.orm import ThreadLocalORMSession
 from alluratest.controller import setup_basic_test, setup_global_objects
 from allura.lib.utils import ConfigProxy
 
-from allura.lib.mail_util import parse_address, parse_message
+from allura.lib.mail_util import parse_address, parse_message, Header
 from allura.lib.exceptions import AddressException
 from allura.tests import decorators as td
 
@@ -88,3 +89,22 @@ class TestReactor(unittest.TestCase):
             if part['payload'] is None: continue
             assert isinstance(part['payload'], unicode)
 
+
+class TestHeader(object):
+
+    @raises(TypeError)
+    def test_bytestring(self):
+        our_header = Header('[asdf2:wiki] Discussion for Home page')
+        assert_equal(str(our_header), '[asdf2:wiki] Discussion for Home page')
+
+    def test_ascii(self):
+        our_header = Header(u'[asdf2:wiki] Discussion for Home page')
+        assert_equal(str(our_header), '[asdf2:wiki] Discussion for Home page')
+
+    def test_utf8(self):
+        our_header = Header(u'теснятся')
+        assert_equal(str(our_header), '=?utf-8?b?0YLQtdGB0L3Rj9GC0YHRjw==?=')
+
+    def test_name_addr(self):
+        our_header = Header(u'"теснятся"', u'<[e-mail]>')
+        assert_equal(str(our_header), '=?utf-8?b?ItGC0LXRgdC90Y/RgtGB0Y8i?= <[e-mail]>')
\ No newline at end of file

--- a/Allura/allura/tests/test_tasks.py
+++ b/Allura/allura/tests/test_tasks.py
@@ -1,10 +1,13 @@
+# -*- coding: utf-8 -*-
 import sys
 import shutil
 import unittest
+from base64 import b64encode
 
 import mock
 from pylons import c, g
-
+from datadiff.tools import assert_equal
+from nose.tools import assert_in
 from ming.orm import FieldProperty, Mapper
 
 from alluratest.controller import setup_basic_test, setup_global_objects
@@ -97,25 +100,56 @@ class TestMailTasks(unittest.TestCase):
         setup_basic_test()
         setup_global_objects()
 
-    def test_send_email(self):
+    # these tests go down through the mail_util.SMTPClient.sendmail method
+    # since usage is generally through the task, and not using mail_util directly
+
+    def test_send_email_ascii_with_user_lookup(self):
         c.user = M.User.by_username('test-admin')
-        with mock.patch.object(mail_tasks.smtp_client, 'sendmail') as f:
+        with mock.patch.object(mail_tasks.smtp_client, '_client') as _client:
             mail_tasks.sendmail(
-                str(c.user._id),
-                [ str(c.user._id) ],
-                'This is a test',
-                '[e-mail]',
-                'Test subject',
-                h.gen_message_id())
-            assert len(f.call_args_list)==3, f.call_args_list
-            args,kwargs  = f.call_args_list[0]
-            assert map(str, args[0]) == [ '"Test Admin" <None>' ]
-            assert str(args[1]) == '"Test Admin" <None>'
-            assert str(args[2]) == '[e-mail]'
-            assert args[3] == 'Test subject'
-            assert '@' in args[4], args[4]
-            assert args[5] == None
-            assert 'This is a test' in str(args[6]), str(args[6])
+                fromaddr=str(c.user._id),
+                destinations=[ str(c.user._id) ],
+                text=u'This is a test',
+                reply_to=u'[e-mail]',
+                subject=u'Test subject',
+                message_id=h.gen_message_id())
+            assert_equal(_client.sendmail.call_count, 1)
+            return_path, rcpts, body = _client.sendmail.call_args[0]
+            body = body.split('\n')
+
+            assert_equal(rcpts, [c.user.get_pref('email_address')])
+            assert_in('Reply-To: [e-mail]', body)
+            assert_in('From: "Test Admin" <[e-mail]>', body)
+            assert_in('Subject: Test subject', body)
+            # plain
+            assert_in('This is a test', body)
+            # html
+            assert_in('<div class="markdown_content"><p>This is a test</p></div>', body)
+
+    def test_send_email_nonascii(self):
+        with mock.patch.object(mail_tasks.smtp_client, '_client') as _client:
+            mail_tasks.sendmail(
+                fromaddr=u'"По" <[e-mail]>',
+                destinations=[ '[e-mail]' ],
+                text=u'Громады стройные теснятся',
+                reply_to=u'[e-mail]',
+                subject=u'По оживлённым берегам',
+                message_id=h.gen_message_id())
+            assert_equal(_client.sendmail.call_count, 1)
+            return_path, rcpts, body = _client.sendmail.call_args[0]
+            body = body.split('\n')
+
+            assert_equal(rcpts, ['[e-mail]'])
+            assert_in('Reply-To: [e-mail]', body)
+
+            # The address portion must not be encoded, only the name portion can be.
+            # Also it is apparently not necessary to have the double-quote separators present
+            #   when the name portion is encoded.  That is, the encoding below is just По and not "По"
+            assert_in('From: =?utf-8?b?0J/Qvg==?= <[e-mail]>', body)
+            assert_in('Subject: =?utf-8?b?0J/QviDQvtC20LjQstC70ZHQvdC90YvQvCDQsdC10YDQtdCz0LDQvA==?=', body)
+            assert_in('Content-Type: text/plain; charset="utf-8"', body)
+            assert_in('Content-Transfer-Encoding: base64', body)
+            assert_in(b64encode(u'Громады стройные теснятся'.encode('utf-8')), body)
 
     @td.with_wiki
     def test_receive_email_ok(self):
@@ -192,7 +226,7 @@ def raise_exc():
         except:
             errs.append(sys.exc_info())
     raise CompoundError(*errs)
-    
+
 class _TestArtifact(M.Artifact):
     _shorthand_id = FieldProperty(str)
     text = FieldProperty(str)

--- a/Allura/allura/websetup/bootstrap.py
+++ b/Allura/allura/websetup/bootstrap.py
@@ -132,7 +132,10 @@ def bootstrap(command, conf, vars):
     # do the minimal needed
     if asbool(conf.get('load_test_data')):
         u_admin = make_user('Test Admin')
-        u_admin.claim_address('[e-mail]')
+        u_admin.preferences = dict(email_address='[e-mail]')
+        u_admin.email_addresses = ['[e-mail]']
+        u_admin.set_password('foo')
+        u_admin.claim_address('[e-mail]')
     else:
         u_admin = make_user('Admin 1', username='admin1')
         # Admin1 is almost root, with admin access for Users and Projects neighborhoods