Merge lp:~cjwatson/launchpad/better-upload-error-notifications into lp:launchpad

[Colin Watson]

Send proper email notifications about most failures to parse the .changes file.

We can now send email notifications to the signer as long as we have a signature, even if the signing key is deactivated. If the .changes file is sufficiently parseable then we'll notify other relevant people as well.

[William Grant (wgrant)]

review: Approve (code)

[lp:~cjwatson/launchpad/better-upload-error-notifications]

18266. By Colin Watson on 2017-09-17

Merge devel.

18267. By Colin Watson on 2017-09-17

GPGKey.active is non-nullable, so simplify SignableTagFile.parse.

18268. By Colin Watson on 2017-09-17

Rename SignableTagFile.verifySignature to SignableTagFile._verifySignature and add a comment about its behaviour with deactivated keys.

=== modified file 'lib/lp/archiveuploader/changesfile.py'
--- lib/lp/archiveuploader/changesfile.py	2017-03-29 09:28:09 +0000
+++ lib/lp/archiveuploader/changesfile.py	2017-09-17 15:23:26 +0000
@@ -84,6 +84,11 @@
     files = None
 
     def __init__(self, filepath, policy, logger):
+        self.filepath = filepath
+        self.policy = policy
+        self.logger = logger
+
+    def parseChanges(self):
         """Process the given changesfile.
 
         Does:
@@ -93,24 +98,25 @@
             * Checks name of changes file
             * Checks signature of changes file
 
-        If any of these checks fail, UploadError is raised, and it's
-        considered a fatal error (no subsequent processing of the upload
-        will be done).
+        If any of these checks fail, UploadError is yielded, and it should
+        be considered a fatal error (no subsequent processing of the upload
+        should be done).
 
         Logger and Policy are instances built in uploadprocessor.py passed
         via NascentUpload class.
         """
-        self.filepath = filepath
-        self.policy = policy
-        self.logger = logger
-
-        self.parse(verify_signature=not policy.unsigned_changes_ok)
+        try:
+            self.parse(verify_signature=not self.policy.unsigned_changes_ok)
+        except UploadError as e:
+            yield e
+            return
 
         for field in self.mandatory_fields:
             if field not in self._dict:
-                raise UploadError(
+                yield UploadError(
                     "Unable to find mandatory field '%s' in the changes "
                     "file." % field)
+                return
 
         try:
             format = float(self._dict["Format"])
@@ -119,7 +125,7 @@
             format = 1.5
 
         if format < 1.5 or format > 2.0:
-            raise UploadError(
+            yield UploadError(
                 "Format out of acceptable range for changes file. Range "
                 "1.5 - 2.0, format %g" % format)
 

=== modified file 'lib/lp/archiveuploader/dscfile.py'
--- lib/lp/archiveuploader/dscfile.py	2017-07-31 11:45:32 +0000
+++ lib/lp/archiveuploader/dscfile.py	2017-09-17 15:23:26 +0000
@@ -142,8 +142,16 @@
                 "Unable to read %s: %s" % (self.filename, error))
 
         if verify_signature:
-            self.signingkey, self.parsed_content = self.verifySignature(
+            # We set self.signingkey regardless of whether the key is
+            # deactivated, since a deactivated key is still good enough for
+            # determining whom to notify, and raising UploadError is enough
+            # to prevent the upload being accepted.
+            self.signingkey, self.parsed_content = self._verifySignature(
                 self.raw_content, self.filepath)
+            if not self.signingkey.active:
+                raise UploadError("File %s is signed with a deactivated key %s"
+                                  % (self.filepath,
+                                     self.signingkey.fingerprint))
         else:
             self.logger.debug("%s can be unsigned." % self.filename)
             self.parsed_content = self.raw_content
@@ -154,7 +162,7 @@
             raise UploadError(
                 "Unable to parse %s: %s" % (self.filename, error))
 
-    def verifySignature(self, content, filename):
+    def _verifySignature(self, content, filename):
         """Verify the signature on the file content.
 
         Raise UploadError if the signing key cannot be found in launchpad
@@ -179,10 +187,6 @@
             raise UploadError("Signing key %s not registered in launchpad."
                               % sig.fingerprint)
 
-        if key.active == False:
-            raise UploadError("File %s is signed with a deactivated key %s"
-                              % (filename, key.fingerprint))
-
         return (key, sig.plain_data)
 
     def parseAddress(self, addr, fieldname="Maintainer"):

=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py	2017-04-21 15:21:27 +0000
+++ lib/lp/archiveuploader/nascentupload.py	2017-09-17 15:23:26 +0000
@@ -142,6 +142,8 @@
         policy = self.policy
         self.logger.debug("Beginning processing.")
 
+        self.run_and_reject_on_error(self.changes.parseChanges)
+
         try:
             policy.setDistroSeriesAndPocket(self.changes.suite_name)
         except NotFoundError:
@@ -780,7 +782,7 @@
                 IDistributionSet)['ubuntu'].currentseries
             return distroseries.createQueueEntry(
                 PackagePublishingPocket.RELEASE,
-                distroseries.main_archive, self.changes.filename,
+                self.policy.archive, self.changes.filename,
                 self.changes.raw_content, signing_key=self.changes.signingkey)
         else:
             return distroseries.createQueueEntry(

=== modified file 'lib/lp/archiveuploader/tests/nascentupload.txt'
--- lib/lp/archiveuploader/tests/nascentupload.txt	2016-01-26 15:47:37 +0000
+++ lib/lp/archiveuploader/tests/nascentupload.txt	2017-09-17 15:23:26 +0000
@@ -39,38 +39,34 @@
     ...     name='anything', distro='ubuntu', distroseries='hoary')
 
 
-Constructing a NascentUpload object
------------------------------------
-
-Constructing a NascentUpload instance verifies that the changes file
-specified exists and tries to build a ChangesFile (see
-doc/nascentuploadfile.txt) object based on that. If anything goes
-wrong during this process UploadError is raised:
+NascentUpload Processing
+------------------------
+
+Processing a NascentUpload consists of building files objects for each
+specified file in the upload, executing all their specific checks and
+collect all errors that may be generated. (see doc/nascentuploadfile.txt)
+
+First, NascentUpload verifies that the changes file specified exist, and
+tries to build a ChangesFile (see doc/nascentuploadfile.txt) object based
+on that.
 
     >>> from lp.services.log.logger import DevNullLogger, FakeLogger
-    >>> NascentUpload.from_changesfile_path(
+
+    >>> nonexistent = NascentUpload.from_changesfile_path(
     ...     datadir("DOES-NOT-EXIST"), buildd_policy, FakeLogger())
+    >>> nonexistent.process()
     Traceback (most recent call last):
     ...
-    UploadError:...
-
-Otherwise a ChangesFile object is ready to use.
+    EarlyReturnUploadError: An error occurred that prevented further
+    processing.
+    >>> nonexistent.is_rejected
+    True
+    >>> print nonexistent.rejection_message
+    Unable to read DOES-NOT-EXIST: ...
 
     >>> quodlibet = NascentUpload.from_changesfile_path(
     ...     datadir("quodlibet_0.13.1-1_i386.changes"),
     ...     anything_policy, DevNullLogger())
-
-    >>> quodlibet.changes
-    <lp.archiveuploader.changesfile.ChangesFile ...>
-
-
-NascentUpload Processing
-------------------------
-
-Processing a NascentUpload consists of building files objects for each
-specified file in the upload, executing all their specific checks and
-collect all errors that may be generated. (see doc/nascentuploadfile.txt)
-
     >>> quodlibet.process()
     >>> for f in quodlibet.changes.files:
     ...     print f.filename, f

=== modified file 'lib/lp/archiveuploader/tests/nascentuploadfile.txt'
--- lib/lp/archiveuploader/tests/nascentuploadfile.txt	2017-08-03 14:26:40 +0000
+++ lib/lp/archiveuploader/tests/nascentuploadfile.txt	2017-09-17 15:23:26 +0000
@@ -72,10 +72,14 @@
     >>> ed_binary_changes = ChangesFile(
     ...     datadir('ed_0.2-20_i386.changes.binary-only'),
     ...     modified_insecure_policy, DevNullLogger())
+    >>> len(list(ed_binary_changes.parseChanges()))
+    0
 
     >>> ed_source_changes = ChangesFile(
     ...     datadir('ed_0.2-20_source.changes'),
     ...     modified_insecure_policy, DevNullLogger())
+    >>> len(list(ed_source_changes.parseChanges()))
+    0
 
 Make sure we are not getting any exceptions due to a malformed changes
 file name.

=== modified file 'lib/lp/archiveuploader/tests/test_changesfile.py'
--- lib/lp/archiveuploader/tests/test_changesfile.py	2017-03-29 09:28:09 +0000
+++ lib/lp/archiveuploader/tests/test_changesfile.py	2017-09-17 15:23:26 +0000
@@ -182,7 +182,10 @@
             changes.dump(changes_fd)
         finally:
             changes_fd.close()
-        return ChangesFile(path, self.policy, self.logger)
+        changesfile = ChangesFile(path, self.policy, self.logger)
+        for error in changesfile.parseChanges():
+            raise error
+        return changesfile
 
     def getBaseChanges(self):
         contents = Changes()
@@ -364,36 +367,39 @@
         import_public_test_keys()
 
     def test_valid_signature_accepted(self):
-        # A correctly signed changes file is excepted, and all its
+        # A correctly signed changes file is accepted, and all its
         # content is parsed.
         path = datadir('signatures/signed.changes')
-        parsed = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
+        changesfile = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
+        self.assertEqual([], list(changesfile.parseChanges()))
         self.assertEqual(
             getUtility(IPersonSet).getByEmail('[e-mail]'),
-            parsed.signer)
+            changesfile.signer)
         expected = "\AFormat: 1.7\n.*foo_1.0-1.diff.gz\Z"
         self.assertTextMatchesExpressionIgnoreWhitespace(
             expected,
-            parsed.parsed_content)
+            changesfile.parsed_content)
 
     def test_no_signature_rejected(self):
         # An unsigned changes file is rejected.
         path = datadir('signatures/unsigned.changes')
-        self.assertRaises(
-            UploadError,
-            ChangesFile, path, InsecureUploadPolicy(), BufferLogger())
+        changesfile = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
+        errors = list(changesfile.parseChanges())
+        self.assertIsInstance(errors[0], UploadError)
+        self.assertEqual(1, len(errors))
 
     def test_prefix_ignored(self):
         # A signed changes file with an unsigned prefix has only the
         # signed part parsed.
         path = datadir('signatures/prefixed.changes')
-        parsed = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
+        changesfile = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
+        self.assertEqual([], list(changesfile.parseChanges()))
         self.assertEqual(
             getUtility(IPersonSet).getByEmail('[e-mail]'),
-            parsed.signer)
+            changesfile.signer)
         expected = "\AFormat: 1.7\n.*foo_1.0-1.diff.gz\Z"
         self.assertTextMatchesExpressionIgnoreWhitespace(
             expected,
-            parsed.parsed_content)
-        self.assertEqual("breezy", parsed.suite_name)
-        self.assertNotIn("evil", parsed.changes_comment)
+            changesfile.parsed_content)
+        self.assertEqual("breezy", changesfile.suite_name)
+        self.assertNotIn("evil", changesfile.changes_comment)

=== modified file 'lib/lp/archiveuploader/tests/test_nascentuploadfile.py'
--- lib/lp/archiveuploader/tests/test_nascentuploadfile.py	2016-11-08 20:22:59 +0000
+++ lib/lp/archiveuploader/tests/test_nascentuploadfile.py	2017-09-17 15:23:26 +0000
@@ -208,7 +208,9 @@
         path = os.path.join(tempdir, filename)
         with open(path, "w") as changes_fd:
             changes.dump(changes_fd)
-        return ChangesFile(path, self.policy, self.logger)
+        changesfile = ChangesFile(path, self.policy, self.logger)
+        self.assertEqual([], list(changesfile.parseChanges()))
+        return changesfile
 
 
 class DSCFileTests(PackageUploadFileTestCase):

=== modified file 'lib/lp/archiveuploader/tests/test_sync_notification.py'
--- lib/lp/archiveuploader/tests/test_sync_notification.py	2015-08-26 13:41:21 +0000
+++ lib/lp/archiveuploader/tests/test_sync_notification.py	2017-09-17 15:23:26 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test notification behaviour for cross-distro package syncs."""
@@ -55,6 +55,7 @@
         self.raw_content = open(file_path).read()
         self.signingkey = None
 
+    parseChanges = FakeMethod([])
     checkFileName = FakeMethod([])
     processAddresses = FakeMethod([])
     processFiles = FakeMethod([])

=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2017-04-21 15:21:27 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2017-09-17 15:23:26 +0000
@@ -51,6 +51,7 @@
     IBuildFarmJobBehaviour,
     )
 from lp.registry.interfaces.distribution import IDistributionSet
+from lp.registry.interfaces.gpg import IGPGKeySet
 from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
@@ -240,7 +241,7 @@
                 excName = excClass.__name__
             else:
                 excName = str(excClass)
-            raise self.failureException, "%s not raised" % excName
+            raise self.failureException("%s not raised" % excName)
 
     def setupBreezy(self, name="breezy", permitted_formats=None):
         """Create a fresh distroseries in ubuntu.
@@ -1955,10 +1956,47 @@
 
         self.assertEqual(UploadStatusEnum.REJECTED, result)
         self.assertLogContains(
-            "INFO Failed to parse changes file")
+            "INFO Not sending rejection notice without a signing key.")
         self.assertEmailQueueLength(0)
         self.assertEqual([], self.oopses)
 
+    def test_deactivated_key_upload_sends_mail(self):
+        # An upload signed with a deactivated key does not OOPS and sends a
+        # rejection email.
+        self.switchToAdmin()
+        fingerprint = "340CA3BB270E2716C9EE0B768E7EB7086C64A8C5"
+        gpgkeyset = getUtility(IGPGKeySet)
+        gpgkeyset.deactivate(gpgkeyset.getByFingerprint(fingerprint))
+        self.switchToUploader()
+
+        uploadprocessor = self.setupBreezyAndGetUploadProcessor()
+        upload_dir = self.queueUpload("netapplet_1.0-1-signed")
+
+        [result] = self.processUpload(uploadprocessor, upload_dir)
+
+        self.assertEqual(UploadStatusEnum.REJECTED, result)
+        base_contents = [
+            "Subject: [ubuntu] netapplet_1.0-1_source.changes (Rejected)",
+            "File %s/netapplet_1.0-1-signed/netapplet_1.0-1_source.changes "
+            "is signed with a deactivated key %s" % (
+                self.incoming_folder, fingerprint),
+            ]
+        expected = []
+        expected.append({
+            "contents": base_contents + [
+                "You are receiving this email because you are the most "
+                    "recent person",
+                "listed in this package's changelog."],
+            "recipient": "[e-mail]",
+            })
+        expected.append({
+            "contents": base_contents + [
+                "You are receiving this email because you made this upload."],
+            "recipient": "[e-mail]",
+            })
+        self.assertEmails(expected)
+        self.assertEqual([], self.oopses)
+
     def test_ddeb_upload_overrides(self):
         # DDEBs should always be overridden to the same values as their
         # counterpart DEB's.

=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py	2017-01-10 15:18:48 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py	2017-09-17 15:23:26 +0000
@@ -338,22 +338,8 @@
         # The path we want for NascentUpload is the path to the folder
         # containing the changes file (and the other files referenced by it).
         changesfile_path = os.path.join(self.upload_path, changes_file)
-        try:
-            upload = NascentUpload.from_changesfile_path(
-                changesfile_path, policy, self.processor.log)
-        except UploadError as e:
-            # We failed to parse the changes file, so we have no key or
-            # Changed-By to notify of the rejection. Just log it and
-            # move on.
-            # XXX wgrant 2011-09-29 bug=499438: With some refactoring we
-            # could do better here: if we have a signature then we have
-            # somebody to email, even if the rest of the file is
-            # corrupt.
-            logger.info(
-                "Failed to parse changes file '%s': %s" % (
-                    os.path.join(self.upload_path, changes_file),
-                    str(e)))
-            return UploadStatusEnum.REJECTED
+        upload = NascentUpload.from_changesfile_path(
+            changesfile_path, policy, self.processor.log)
 
         # Reject source upload to buildd upload paths.
         first_path = relative_path.split(os.path.sep)[0]
@@ -411,7 +397,16 @@
                 notify = False
             if upload.is_rejected:
                 result = UploadStatusEnum.REJECTED
-                upload.do_reject(notify)
+                if upload.changes.parsed_content is not None:
+                    # We got past the point of checking any required
+                    # signature, so we can do a proper rejection.
+                    upload.do_reject(notify)
+                else:
+                    # The upload required a signature and either didn't have
+                    # one or we failed to verify it, so we have nobody to
+                    # notify.  Just log it and move on.
+                    logger.info(
+                        "Not sending rejection notice without a signing key.")
                 self.processor.ztm.abort()
             else:
                 successful = self._acceptUpload(upload, notify)

=== modified file 'lib/lp/soyuz/doc/distroseriesqueue-translations.txt'
--- lib/lp/soyuz/doc/distroseriesqueue-translations.txt	2015-09-04 12:19:07 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue-translations.txt	2017-09-17 15:23:26 +0000
@@ -87,10 +87,9 @@
     >>> pmount_upload = NascentUpload.from_changesfile_path(
     ...     datadir('pmount_0.9.7-2ubuntu2_amd64.changes'),
     ...     buildd_policy, FakeLogger())
-    DEBUG pmount_0.9.7-2ubuntu2_amd64.changes can be unsigned.
-
     >>> pmount_upload.process(build=build)
     DEBUG Beginning processing.
+    DEBUG pmount_0.9.7-2ubuntu2_amd64.changes can be unsigned.
     DEBUG Verifying the changes file.
     DEBUG Verifying files in upload.
     DEBUG Verifying binary pmount_0.9.7-2ubuntu2_amd64.deb