Merge ~ilasc/launchpad:bug-1910403 into launchpad:master

[Ioana Lasc]

UserUploadHandler fails without Maintainer email

[Ioana Lasc (ilasc)]

I'm expecting the added test to fail with the stack in the bug 1910403 but it fails with the following stack:

getVerifiedSignature, handler.py:253
getVerifiedSignatureResilient, handler.py:147
_verifySignature, dscfile.py:190
parse, dscfile.py:153
parseChanges, changesfile.py:109
run_and_collect_errors, nascentupload.py:434
run_and_reject_on_error, nascentupload.py:445
process, nascentupload.py:145
_processUpload, uploadprocessor.py:538
processChangesFile, uploadprocessor.py:368
processUpload, test_uploadprocessor.py:337
testUploadWithoutMaintainerEmail, test_uploadprocessor.py:553
_run_test_method, testcase.py:723
_run_user, runtest.py:191
_run_core, runtest.py:144
_run_prepared_result, runtest.py:108
_run_one, runtest.py:94
run, runtest.py:80
run, testcase.py:675
__call__, case.py:673
run_tests, runner.py:396
run_layer, runner.py:474
run_tests, runner.py:293
run, runner.py:191
main, test.py:275
<module>, test:8

And the final exception (lp.services.gpg.interfaces.GPGVerificationError) is visible to the "user/developer/test runner" as :
[u"GPG verification of /tmp/tmpX1xckK/incoming/bar_1.0-1/bar_1.0-1_source.changes failed: (7, 8, u'Bad signature')"]

Not sure how we get from here to the exception stack in the Bug assessment.

[Ioana Lasc (ilasc)]

If however we do have the Maintainer email address in the changes file and add a non ASCII character in the email template params, we see this failure stack, again associated with failing to verify the signature:

processChangesFile, uploadprocessor.py:409
processUpload, test_uploadprocessor.py:337
testUploadWithoutMaintainerEmail, test_uploadprocessor.py:553
_run_test_method, testcase.py:723
_run_user, runtest.py:191
_run_core, runtest.py:144
_run_prepared_result, runtest.py:108
_run_one, runtest.py:94
run, runtest.py:80
run, testcase.py:675
__call__, case.py:673
run_tests, runner.py:396
run_layer, runner.py:474
run_tests, runner.py:293
run, runner.py:191
main, test.py:275
<module>, test:8

[Ioana Lasc (ilasc)]

OK, now reproduced the exact same stack trace from the OOPS through unit test "testParseMaintainerNonASCII" and for the same reason captured by bug 1910403: non ASCII characters in the Maintainer field which will ultimately be a part of the email template parameters along with other fields in lp.services.mail.basemailer, line 185.

I would think we would see the same in the other test "testUploadWithoutNonASCII" -> this however is failing signature verification in the same points pasted in stack traces in my previous comments. I don't understand why it's failing differently there, but I also don't understand that area of the code enough yet and I'm probably not setting up the test correctly to begin with,

[Ioana Lasc (ilasc)]

MP is now ready for first review.

[Colin Watson (cjwatson)]

You can reduce the original OOPS to whether an exception e from `parse_maintainer_bytes` can be rendered as text using `six.text_type(e)`; that's not literally what the email layer does, but it's close enough. This patch doesn't fix that. (In fact, while the cleanup in `ParseMaintError` looks reasonable, I don't think it fixes anything, as the new test you've added passes with or without that cleanup.)

>>> import six
>>> from lp.archiveuploader.utils import parse_maintainer_bytes
>>> try:
... parse_maintainer_bytes(b'No\xc3\xa8l K\xc3\xb6the', 'Maintainer')
... except Exception as e:
... pass
>>> six.text_type(e)
Traceback (most recent call last):
  File "<console>", line 1, in <module>
UnicodeDecodeError: 'ascii' codec can't decode byte 0xc3 in position 2: ordinal not in range(128)

Consider instead just constructing the `ParseMaintError` exceptions raised by `parse_maintainer` as Unicode rather than encoding text to UTF-8 there. I think that should work better.

You may also want to fold the test for this into a simple modification to `testParseMaintainerRaises`, rather than adding a new test case. It looks as though a change to that test will be needed for a correct fix anyway.

review: Needs Fixing

[Ioana Lasc (ilasc)]

Colin thank you for the details!
Incorporated all suggested changes, MP now ready for another look.

[Colin Watson (cjwatson)]

review: Approve

diff --git a/lib/lp/archiveuploader/tests/test_utils.py b/lib/lp/archiveuploader/tests/test_utils.py
index e4d11ef..52caef5 100755
--- a/lib/lp/archiveuploader/tests/test_utils.py
+++ b/lib/lp/archiveuploader/tests/test_utils.py
@@ -6,9 +6,8 @@
 from __future__ import absolute_import, print_function, unicode_literals
 
 import os
-import re
 
-from testtools.testcase import ExpectedException
+import six
 
 from lp.archiveuploader.tests import datadir
 from lp.archiveuploader.utils import (
@@ -16,6 +15,7 @@ from lp.archiveuploader.utils import (
     determine_source_file_type,
     DpkgSourceError,
     extract_dpkg_source,
+    ParseMaintError,
     re_isadeb,
     re_issource,
     )
@@ -234,17 +234,32 @@ class TestUtilities(TestCase):
         """
         from lp.archiveuploader.utils import (
             parse_maintainer_bytes,
-            ParseMaintError,
             )
+
         cases = (
-            "James Troup",
-            "James Troup <james>",
-            "James Troup <[e-mail]",
-            b"No\xc3\xa8l K\xc3\xb6the")
+            ("James Troup",
+             'James Troup: no @ found in email address part.'),
+
+            ("James Troup <james>",
+             'James Troup <james>: no @ found in email address part.'),
+
+            ("James Troup <[e-mail]",
+             ("James Troup <[e-mail]: "
+              "doesn't parse as a valid Maintainer field.")),
+
+            (b"No\xc3\xa8l K\xc3\xb6the",
+             (b'No\xc3\xa8l K\xc3\xb6the: '
+              b'no @ found in email address '
+              b'part.').decode('utf-8')),
+        )
+
         for case in cases:
-            with ExpectedException(
-                    ParseMaintError, b'^%s: ' % re.escape(case)):
-                parse_maintainer_bytes(case, 'Maintainer')
+            try:
+                parse_maintainer_bytes(case[0], 'Maintainer')
+            except ParseMaintError as e:
+                self.assertEqual(case[1], six.text_type(e))
+            else:
+                self.fail('ParseMaintError not raised')
 
 
 class TestFilenameRegularExpressions(TestCase):
diff --git a/lib/lp/archiveuploader/utils.py b/lib/lp/archiveuploader/utils.py
index 8b4cf73..4168d96 100644
--- a/lib/lp/archiveuploader/utils.py
+++ b/lib/lp/archiveuploader/utils.py
@@ -171,16 +171,8 @@ def extract_component_from_section(section, default_component="main"):
 
 class ParseMaintError(Exception):
     """Exception raised for errors in parsing a maintainer field.
-
-    Attributes:
-       message -- explanation of the error
     """
 
-    def __init__(self, message):
-        Exception.__init__(self)
-        self.args = (message, )
-        self.message = message
-
 
 def parse_maintainer(maintainer, field_name="Maintainer"):
     """Parses a Maintainer or Changed-By field into the name and address.
@@ -200,9 +192,8 @@ def parse_maintainer(maintainer, field_name="Maintainer"):
     else:
         m = re_parse_maintainer.match(maintainer)
         if not m:
-            raise ParseMaintError(
-                "%s: doesn't parse as a valid %s field."
-                % (maintainer.encode("utf-8"), field_name))
+            raise ParseMaintError("%s: doesn't parse as a valid %s field."
+                                  % (maintainer, field_name))
         name = m.group(1)
         email = m.group(2)
         # Just in case the maintainer ended up with nested angles; check...
@@ -210,9 +201,8 @@ def parse_maintainer(maintainer, field_name="Maintainer"):
             email = email[1:]
 
     if email.find(u"@") == -1 and email.find(u"buildd_") != 0:
-        raise ParseMaintError(
-            "%s: no @ found in email address part." %
-            maintainer.encode("utf-8"))
+        raise ParseMaintError("%s: no @ found in email address part."
+                              % maintainer)
 
     return (name, email)