Mail improvements:

- Implement new config option in mail-section "ignore_alternatives" to
  ignore alternatives in a multipart/alternative mail. The *last*
  text/plain part of the *first* multipart/alternative is used as the
  message, if ignore_alternatives is set all other alternative parts of
  the first multipart/alternative that contained a text/plain part are
  ignored. Other multipart/alternative or other multipart are attached
  as before. This fixes [SF#959811] "Multipart/alternative handling
  considered bad". Note that this also changes which text/plain part is
  attached as the message if there are several text/plain parts in a
  multipart: Previously the *first* text/plain would be attached. Now we
  attach the *last* one, this is more in line with rfc 2046, sec. 5.1.4.
  according to Philipp Gortan.
- Fix bug in attachment of text parts: If there are multiple text/plain
  parts in a nested multipart, the previous code would attach the
  multipart serialisation instead of the text/plain serialisation as a
  file to the issue in some cases.
- Add regression tests for the new config-option and bug-fixes above.

Author: Ralf Schlatterbeck

CHANGES.txt

@@ -4,6 +4,9 @@ are given with the most recent entry first.
 2007-11-?? 1.4.1
 Fixed:
 - Removed some metakit references
+- New config option in mail section: ignore_alternatives allows to
+  ignore alternatives besides the text/plain part used for the content
+  of a message in multipart/alternative attachments.
 
 
 2007-11-04 1.4.0

roundup/configuration.py

@@ -1,6 +1,6 @@
 # Roundup Issue Tracker configuration support
 #
-# $Id: configuration.py,v 1.49 2007-09-26 03:20:21 jpend Exp $
+# $Id: configuration.py,v 1.50 2007-11-14 14:57:47 schlatterbeck Exp $
 #
 __docformat__ = "restructuredtext"
 
@@ -716,6 +716,12 @@ def str2value(self, value):
             "Regular expression matching end of line."),
         (RegExpOption, "blankline_re", r"[\r\n]+\s*[\r\n]+",
             "Regular expression matching a blank line."),
+        (BooleanOption, "ignore_alternatives", "no",
+            "When parsing incoming mails, roundup uses the first\n"
+            "text/plain part it finds. If this part is inside a\n"
+            "multipart/alternative, and this option is set, all other\n"
+            "parts of the multipart/alternative are ignored. The default\n"
+            "is to keep all parts and attach them to the issue."),
     ), "Roundup Mail Gateway options"),
     ("pgp", (
         (BooleanOption, "enable", "no",

roundup/mailgw.py

@@ -73,7 +73,7 @@ class node. Any parts of other types are each stored in separate files
 an exception, the original message is bounced back to the sender with the
 explanatory message given in the exception.
 
-$Id: mailgw.py,v 1.192 2007-09-26 03:20:21 jpend Exp $
+$Id: mailgw.py,v 1.193 2007-11-14 14:57:47 schlatterbeck Exp $
 """
 __docformat__ = 'restructuredtext'
 
@@ -346,26 +346,49 @@ def getbody(self):
     # multipart/form-data:
     #   For web forms only.
 
-    def extract_content(self, parent_type=None):
-        """Extract the body and the attachments recursively."""
+    def extract_content(self, parent_type=None, ignore_alternatives = False):
+        """Extract the body and the attachments recursively.
+        
+           If the content is hidden inside a multipart/alternative part,
+           we use the *last* text/plain part of the *first*
+           multipart/alternative in the whole message.
+        """
         content_type = self.gettype()
         content = None
         attachments = []
 
         if content_type == 'text/plain':
             content = self.getbody()
         elif content_type[:10] == 'multipart/':
+            content_found = bool (content)
+            ig = ignore_alternatives and not content_found
             for part in self.getparts():
-                new_content, new_attach = part.extract_content(content_type)
+                new_content, new_attach = part.extract_content(content_type,
+                    not content and ig)
 
                 # If we haven't found a text/plain part yet, take this one,
                 # otherwise make it an attachment.
                 if not content:
                     content = new_content
+                    cpart   = part
                 elif new_content:
-                    attachments.append(part.as_attachment())
+                    if content_found or content_type != 'multipart/alternative':
+                        attachments.append(part.text_as_attachment())
+                    else:
+                        # if we have found a text/plain in the current
+                        # multipart/alternative and find another one, we
+                        # use the first as an attachment (if configured)
+                        # and use the second one because rfc 2046, sec.
+                        # 5.1.4. specifies that later parts are better
+                        # (thanks to Philipp Gortan for pointing this
+                        # out)
+                        attachments.append(cpart.text_as_attachment())
+                        content = new_content
+                        cpart   = part
 
                 attachments.extend(new_attach)
+            if ig and content_type == 'multipart/alternative' and content:
+                attachments = []
         elif (parent_type == 'multipart/signed' and
               content_type == 'application/pgp-signature'):
             # ignore it so it won't be saved as an attachment
@@ -374,6 +397,20 @@ def extract_content(self, parent_type=None):
             attachments.append(self.as_attachment())
         return content, attachments
 
+    def text_as_attachment(self):
+        """Return first text/plain part as Message"""
+        if not self.gettype().startswith ('multipart/'):
+            return self.as_attachment()
+        for part in self.getparts():
+            content_type = part.gettype()
+            if content_type == 'text/plain':
+                return part.as_attachment()
+            elif content_type.startswith ('multipart/'):
+                p = part.text_as_attachment()
+                if p:
+                    return p
+        return None
+
     def as_attachment(self):
         """Return this message as an attachment."""
         return (self.getname(), self.gettype(), self.getbody())
@@ -1204,7 +1241,8 @@ def pgp_role():
 This tracker has been configured to require all email be PGP signed or
 encrypted.""")
         # now handle the body - find the message
-        content, attachments = message.extract_content()
+        ig = self.instance.config.MAILGW_IGNORE_ALTERNATIVES
+        content, attachments = message.extract_content(ignore_alternatives = ig)
         if content is None:
             raise MailUsageError, _("""
 Roundup requires the submission to be plain text. The message parser could

test/test_mailgw.py

@@ -8,7 +8,7 @@
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
 #
-# $Id: test_mailgw.py,v 1.90 2007-11-04 06:11:19 richard Exp $
+# $Id: test_mailgw.py,v 1.91 2007-11-14 14:57:47 schlatterbeck Exp $
 
 # TODO: test bcc
 
@@ -340,14 +340,106 @@ def testNewIssueNoAuthorEmail(self):
 _______________________________________________________________________
 ''')
 
-    # BUG
-    # def testMultipart(self):
-    #         '''With more than one part'''
-    #        see MultipartEnc tests: but if there is more than one part
-    #        we return a multipart/mixed and the boundary contains
-    #        the ip address of the test machine.
+    multipart_msg = '''Content-Type: text/plain;
+  charset="iso-8859-1"
+From: mary <[email protected]>
+To: [email protected]
+Message-Id: <followup_dummy_id>
+In-Reply-To: <dummy_test_message_id>
+Subject: [issue1] Testing...
+Content-Type: multipart/mixed; boundary="bxyzzy"
+Content-Disposition: inline
+
+
+--bxyzzy
+Content-Type: multipart/alternative; boundary="bCsyhTFzCvuiizWE"
+Content-Disposition: inline
+
+--bCsyhTFzCvuiizWE
+Content-Type: text/plain; charset=us-ascii
+Content-Disposition: inline
+
+test attachment first text/plain
+
+--bCsyhTFzCvuiizWE
+Content-Type: application/octet-stream
+Content-Disposition: attachment; filename="first.dvi"
+Content-Transfer-Encoding: base64
+
+SnVzdCBhIHRlc3QgAQo=
+
+--bCsyhTFzCvuiizWE
+Content-Type: text/plain; charset=us-ascii
+Content-Disposition: inline
+
+test attachment second text/plain
 
-    # BUG should test some binary attamchent too.
+--bCsyhTFzCvuiizWE
+Content-Type: text/html
+Content-Disposition: inline
+
+<html>
+to be ignored.
+</html>
+
+--bCsyhTFzCvuiizWE--
+
+--bxyzzy
+Content-Type: multipart/alternative; boundary="bCsyhTFzCvuiizWF"
+Content-Disposition: inline
+
+--bCsyhTFzCvuiizWF
+Content-Type: text/plain; charset=us-ascii
+Content-Disposition: inline
+
+test attachment third text/plain
+
+--bCsyhTFzCvuiizWF
+Content-Type: application/octet-stream
+Content-Disposition: attachment; filename="second.dvi"
+Content-Transfer-Encoding: base64
+
+SnVzdCBhIHRlc3QK
+
+--bCsyhTFzCvuiizWF--
+
+--bxyzzy--
+'''
+
+    def testMultipartKeepAlternatives(self):
+        self.doNewIssue()
+        self._handle_mail(self.multipart_msg)
+        messages = self.db.issue.get('1', 'messages')
+        messages.sort()
+        msg = self.db.msg.getnode (messages[-1])
+        assert(len(msg.files) == 5)
+        names = {0 : 'first.dvi', 4 : 'second.dvi'}
+        content = {3 : 'test attachment third text/plain\n',
+                   4 : 'Just a test\n'}
+        for n, id in enumerate (msg.files):
+            f = self.db.file.getnode (id)
+            self.assertEqual(f.name, names.get (n, 'unnamed'))
+            if n in content :
+                self.assertEqual(f.content, content [n])
+        self.assertEqual(msg.content, 'test attachment second text/plain')
+
+    def testMultipartDropAlternatives(self):
+        self.doNewIssue()
+        self.db.config.MAILGW_IGNORE_ALTERNATIVES = True
+        self._handle_mail(self.multipart_msg)
+        messages = self.db.issue.get('1', 'messages')
+        messages.sort()
+        msg = self.db.msg.getnode (messages[-1])
+        assert(len(msg.files) == 2)
+        names = {1 : 'second.dvi'}
+        content = {0 : 'test attachment third text/plain\n',
+                   1 : 'Just a test\n'}
+        for n, id in enumerate (msg.files):
+            f = self.db.file.getnode (id)
+            self.assertEqual(f.name, names.get (n, 'unnamed'))
+            if n in content :
+                self.assertEqual(f.content, content [n])
+        self.assertEqual(msg.content, 'test attachment second text/plain')
 
     def testSimpleFollowup(self):
         self.doNewIssue()
@@ -1068,15 +1160,17 @@ def testContentDisposition(self):
 --bCsyhTFzCvuiizWE
 Content-Type: application/octet-stream
 Content-Disposition: attachment; filename="main.dvi"
+Content-Transfer-Encoding: base64
 
-xxxxxx
+SnVzdCBhIHRlc3QgAQo=
 
 --bCsyhTFzCvuiizWE--
 ''')
         messages = self.db.issue.get('1', 'messages')
         messages.sort()
-        file = self.db.msg.get(messages[-1], 'files')[0]
-        self.assertEqual(self.db.file.get(file, 'name'), 'main.dvi')
+        file = self.db.file.getnode (self.db.msg.get(messages[-1], 'files')[0])
+        self.assertEqual(file.name, 'main.dvi')
+        self.assertEqual(file.content, 'Just a test \001')
 
     def testFollowupStupidQuoting(self):
         self.doNewIssue()