launchpad-reviewers team mailing list archive

[Martin Pool <martinpool@xxxxxxxxx>]

Martin Pool has proposed merging lp:~mbp/launchpad/790902-mail-librarian into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~mbp/launchpad/790902-mail-librarian/+merge/63816

Launchpad stores mails into the librarian for the sake of debugging, and so that mailman and code review can later get back the exact original mail.

At the moment there are two functions that do this; this unifies them.

Also, one of them tries to obfuscate the message id, but in a not particularly useful way since it is deterministic.  This makes it just random instead.  (Bug 790902.)
-- 
https://code.launchpad.net/~mbp/launchpad/790902-mail-librarian/+merge/63816
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/790902-mail-librarian into lp:launchpad.

=== modified file 'lib/canonical/launchpad/helpers.py'
--- lib/canonical/launchpad/helpers.py	2011-04-07 04:58:20 +0000
+++ lib/canonical/launchpad/helpers.py	2011-06-08 05:48:32 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Various functions and classes that are useful across different parts of
@@ -11,7 +11,6 @@
 __metaclass__ = type
 
 from difflib import unified_diff
-import hashlib
 import os
 import random
 import re
@@ -28,7 +27,6 @@
     IRequestLocalLanguages,
     IRequestPreferredLanguages,
     )
-from lp.services.utils import compress_hash
 
 
 def text_replaced(text, replacements, _cache={}):
@@ -413,14 +411,6 @@
     return "application/octet-stream"
 
 
-def get_filename_from_message_id(message_id):
-    """Returns a librarian filename based on the email message_id.
-
-    It generates a file name that's not easily guessable.
-    """
-    return '%s.msg' % compress_hash(hashlib.sha1(message_id))
-
-
 def intOrZero(value):
     """Return int(value) or 0 if the conversion fails.
 

=== modified file 'lib/canonical/launchpad/mail/helpers.py'
--- lib/canonical/launchpad/mail/helpers.py	2011-02-01 04:57:42 +0000
+++ lib/canonical/launchpad/mail/helpers.py	2011-06-08 05:48:32 +0000
@@ -1,11 +1,13 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
 
+from cStringIO import StringIO as cStringIO
 import os.path
 import re
 import time
+from uuid import uuid1
 
 from zope.component import getUtility
 
@@ -18,10 +20,10 @@
     EmailProcessingError,
     IWeaklyAuthenticatedPrincipal,
     )
+from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
 from canonical.launchpad.webapp import canonical_url
 from canonical.launchpad.webapp.interaction import get_current_principal
 from canonical.launchpad.webapp.interfaces import ILaunchBag
-from lp.bugs.enum import BugNotificationLevel
 from lp.registry.vocabularies import ValidPersonOrTeamVocabulary
 
 
@@ -257,3 +259,20 @@
             or timestamp > ten_minutes_in_the_future):
         error_message = get_error_message(error_template, context=context)
         raise IncomingEmailError(error_message)
+
+
+def save_mail_to_librarian(raw_mail):
+    """Save the message to the librarian.
+
+    It can be referenced from errors, and also accessed by code that needs to
+    get back the exact original form.
+    """
+    # File the raw_mail in the Librarian.  We generate a filename to avoid
+    # people guessing the URL.  We don't want URLs to private bug messages to
+    # be guessable for example.
+    file_name = str(uuid1()) + '.txt'
+    file_alias = getUtility(ILibraryFileAliasSet).create(
+            file_name,
+            len(raw_mail),
+            cStringIO(raw_mail), 'message/rfc822')
+    return file_alias

=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py	2011-06-07 19:12:20 +0000
+++ lib/lp/services/mail/incoming.py	2011-06-08 05:48:32 +0000
@@ -16,7 +16,6 @@
 import logging
 import re
 import sys
-from uuid import uuid1
 
 import dkim
 import dns.exception
@@ -32,11 +31,13 @@
     GPGVerificationError,
     IGPGHandler,
     )
-from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
 from canonical.launchpad.interfaces.mail import IWeaklyAuthenticatedPrincipal
 from canonical.launchpad.interfaces.mailbox import IMailBox
 from canonical.launchpad.mail.commands import get_error_message
-from canonical.launchpad.mail.helpers import ensure_sane_signature_timestamp
+from canonical.launchpad.mail.helpers import (
+    ensure_sane_signature_timestamp,
+    save_mail_to_librarian,
+    )
 from canonical.launchpad.mailnotification import (
     send_process_error_notification,
     )
@@ -329,12 +330,18 @@
     try:
         for mail_id, raw_mail in mailbox.items():
             log.info("Processing mail %s" % mail_id)
+            trans.begin()
             try:
-                file_alias = save_mail_to_librarian(trans, log, raw_mail)
+                file_alias = save_mail_to_librarian(raw_mail)
                 # Let's save the url of the file alias, otherwise we might not
                 # be able to access it later if we get a DB exception.
                 file_alias_url = file_alias.http_url
                 log.debug('Uploaded mail to librarian %s' % (file_alias_url,))
+                # If something goes wrong when handling the mail, the
+                # transaction will be aborted. Therefore we need to commit the
+                # transaction now, to ensure that the mail gets stored in the
+                # Librarian.
+                trans.commit()
             except UploadFailed:
                 # Something went wrong in the Librarian. It could be that it's
                 # not running, but not necessarily. Log the error and skip the
@@ -400,22 +407,6 @@
     trans.commit()
 
 
-def save_mail_to_librarian(trans, log, raw_mail):
-    """Save the message to the librarian, for reference from errors."""
-    trans.begin()
-    # File the raw_mail in the Librarian
-    file_name = str(uuid1()) + '.txt'
-    file_alias = getUtility(ILibraryFileAliasSet).create(
-            file_name, len(raw_mail),
-            cStringIO(raw_mail), 'message/rfc822')
-    # If something goes wrong when handling the mail, the
-    # transaction will be aborted. Therefore we need to commit the
-    # transaction now, to ensure that the mail gets stored in the
-    # Librarian.
-    trans.commit()
-    return file_alias
-
-
 def handle_one_mail(log, mail, file_alias, file_alias_url,
                     signature_timestamp_checker):
     """Process one message.

=== modified file 'lib/lp/services/messages/model/message.py'
--- lib/lp/services/messages/model/message.py	2011-05-31 20:59:10 +0000
+++ lib/lp/services/messages/model/message.py	2011-06-08 05:48:32 +0000
@@ -62,7 +62,6 @@
 from canonical.database.datetimecol import UtcDateTimeCol
 from canonical.database.enumcol import EnumCol
 from canonical.database.sqlbase import SQLBase
-from canonical.launchpad.helpers import get_filename_from_message_id
 from canonical.launchpad.interfaces.librarian import (
     ILibraryFileAliasSet,
     )
@@ -76,7 +75,9 @@
     IUserToUserEmail,
     UnknownSender,
     )
-from canonical.launchpad.mail import signed_message_from_string
+from canonical.launchpad.mail import (
+    signed_message_from_string,
+    )
 from lp.app.errors import NotFoundError
 from lp.registry.interfaces.person import (
     IPersonSet,
@@ -306,14 +307,11 @@
         # already in there.
         file_alias_set = getUtility(ILibraryFileAliasSet)  # Reused later
         if filealias is None:
-            # We generate a filename to avoid people guessing the URL.
-            # We don't want URLs to private bug messages to be guessable
-            # for example.
-            raw_filename = get_filename_from_message_id(
-                parsed_message['message-id'])
-            raw_email_message = file_alias_set.create(
-                    raw_filename, len(email_message),
-                    cStringIO(email_message), 'message/rfc822')
+            # Avoid circular import.
+            from canonical.launchpad.mail.helpers import (
+                save_mail_to_librarian,
+                )
+            raw_email_message = save_mail_to_librarian(email_message)
         else:
             raw_email_message = filealias
 
@@ -473,8 +471,7 @@
                         name=filename,
                         size=len(content),
                         file=cStringIO(content),
-                        contentType=content_type
-                        )
+                        contentType=content_type)
                     MessageChunk(message=message, sequence=sequence,
                                  blob=blob)
                     sequence += 1
@@ -551,8 +548,7 @@
 
     blob = ForeignKey(
         foreignKey='LibraryFileAlias', dbName='blob', notNull=False,
-        default=None
-        )
+        default=None)
 
     def __unicode__(self):
         """Return a text representation of this chunk.
@@ -567,8 +563,7 @@
             return (
                 "Attachment: %s\n"
                 "Type:       %s\n"
-                "URL:        %s" % (blob.filename, blob.mimetype, blob.url)
-                )
+                "URL:        %s" % (blob.filename, blob.mimetype, blob.url))
 
 
 class UserToUserEmail(Storm):