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

Proposed by Colin Watson on 2016-11-17
Status: Merged
Merged at revision: 18472
Proposed branch: lp:~cjwatson/launchpad/better-upload-error-notifications
Merge into: lp:launchpad
Diff against target: 438 lines (+129/-76)
11 files modified
lib/lp/archiveuploader/changesfile.py (+16/-10)
lib/lp/archiveuploader/dscfile.py (+10/-6)
lib/lp/archiveuploader/nascentupload.py (+3/-1)
lib/lp/archiveuploader/tests/nascentupload.txt (+19/-23)
lib/lp/archiveuploader/tests/nascentuploadfile.txt (+4/-0)
lib/lp/archiveuploader/tests/test_changesfile.py (+19/-13)
lib/lp/archiveuploader/tests/test_nascentuploadfile.py (+3/-1)
lib/lp/archiveuploader/tests/test_sync_notification.py (+2/-1)
lib/lp/archiveuploader/tests/test_uploadprocessor.py (+40/-2)
lib/lp/archiveuploader/uploadprocessor.py (+12/-17)
lib/lp/soyuz/doc/distroseriesqueue-translations.txt (+1/-2)
To merge this branch: bzr merge lp:~cjwatson/launchpad/better-upload-error-notifications
Reviewer Review Type Date Requested Status
William Grant code 2016-11-17 Approve on 2017-07-24
Review via email: mp+311179@code.launchpad.net

Commit message

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

Description of the change

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.

To post a comment you must log in.
William Grant (wgrant) :
review: Approve (code)
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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/archiveuploader/changesfile.py'
2--- lib/lp/archiveuploader/changesfile.py 2017-03-29 09:28:09 +0000
3+++ lib/lp/archiveuploader/changesfile.py 2017-09-17 15:23:26 +0000
4@@ -84,6 +84,11 @@
5 files = None
6
7 def __init__(self, filepath, policy, logger):
8+ self.filepath = filepath
9+ self.policy = policy
10+ self.logger = logger
11+
12+ def parseChanges(self):
13 """Process the given changesfile.
14
15 Does:
16@@ -93,24 +98,25 @@
17 * Checks name of changes file
18 * Checks signature of changes file
19
20- If any of these checks fail, UploadError is raised, and it's
21- considered a fatal error (no subsequent processing of the upload
22- will be done).
23+ If any of these checks fail, UploadError is yielded, and it should
24+ be considered a fatal error (no subsequent processing of the upload
25+ should be done).
26
27 Logger and Policy are instances built in uploadprocessor.py passed
28 via NascentUpload class.
29 """
30- self.filepath = filepath
31- self.policy = policy
32- self.logger = logger
33-
34- self.parse(verify_signature=not policy.unsigned_changes_ok)
35+ try:
36+ self.parse(verify_signature=not self.policy.unsigned_changes_ok)
37+ except UploadError as e:
38+ yield e
39+ return
40
41 for field in self.mandatory_fields:
42 if field not in self._dict:
43- raise UploadError(
44+ yield UploadError(
45 "Unable to find mandatory field '%s' in the changes "
46 "file." % field)
47+ return
48
49 try:
50 format = float(self._dict["Format"])
51@@ -119,7 +125,7 @@
52 format = 1.5
53
54 if format < 1.5 or format > 2.0:
55- raise UploadError(
56+ yield UploadError(
57 "Format out of acceptable range for changes file. Range "
58 "1.5 - 2.0, format %g" % format)
59
60
61=== modified file 'lib/lp/archiveuploader/dscfile.py'
62--- lib/lp/archiveuploader/dscfile.py 2017-07-31 11:45:32 +0000
63+++ lib/lp/archiveuploader/dscfile.py 2017-09-17 15:23:26 +0000
64@@ -142,8 +142,16 @@
65 "Unable to read %s: %s" % (self.filename, error))
66
67 if verify_signature:
68- self.signingkey, self.parsed_content = self.verifySignature(
69+ # We set self.signingkey regardless of whether the key is
70+ # deactivated, since a deactivated key is still good enough for
71+ # determining whom to notify, and raising UploadError is enough
72+ # to prevent the upload being accepted.
73+ self.signingkey, self.parsed_content = self._verifySignature(
74 self.raw_content, self.filepath)
75+ if not self.signingkey.active:
76+ raise UploadError("File %s is signed with a deactivated key %s"
77+ % (self.filepath,
78+ self.signingkey.fingerprint))
79 else:
80 self.logger.debug("%s can be unsigned." % self.filename)
81 self.parsed_content = self.raw_content
82@@ -154,7 +162,7 @@
83 raise UploadError(
84 "Unable to parse %s: %s" % (self.filename, error))
85
86- def verifySignature(self, content, filename):
87+ def _verifySignature(self, content, filename):
88 """Verify the signature on the file content.
89
90 Raise UploadError if the signing key cannot be found in launchpad
91@@ -179,10 +187,6 @@
92 raise UploadError("Signing key %s not registered in launchpad."
93 % sig.fingerprint)
94
95- if key.active == False:
96- raise UploadError("File %s is signed with a deactivated key %s"
97- % (filename, key.fingerprint))
98-
99 return (key, sig.plain_data)
100
101 def parseAddress(self, addr, fieldname="Maintainer"):
102
103=== modified file 'lib/lp/archiveuploader/nascentupload.py'
104--- lib/lp/archiveuploader/nascentupload.py 2017-04-21 15:21:27 +0000
105+++ lib/lp/archiveuploader/nascentupload.py 2017-09-17 15:23:26 +0000
106@@ -142,6 +142,8 @@
107 policy = self.policy
108 self.logger.debug("Beginning processing.")
109
110+ self.run_and_reject_on_error(self.changes.parseChanges)
111+
112 try:
113 policy.setDistroSeriesAndPocket(self.changes.suite_name)
114 except NotFoundError:
115@@ -780,7 +782,7 @@
116 IDistributionSet)['ubuntu'].currentseries
117 return distroseries.createQueueEntry(
118 PackagePublishingPocket.RELEASE,
119- distroseries.main_archive, self.changes.filename,
120+ self.policy.archive, self.changes.filename,
121 self.changes.raw_content, signing_key=self.changes.signingkey)
122 else:
123 return distroseries.createQueueEntry(
124
125=== modified file 'lib/lp/archiveuploader/tests/nascentupload.txt'
126--- lib/lp/archiveuploader/tests/nascentupload.txt 2016-01-26 15:47:37 +0000
127+++ lib/lp/archiveuploader/tests/nascentupload.txt 2017-09-17 15:23:26 +0000
128@@ -39,38 +39,34 @@
129 ... name='anything', distro='ubuntu', distroseries='hoary')
130
131
132-Constructing a NascentUpload object
133------------------------------------
134-
135-Constructing a NascentUpload instance verifies that the changes file
136-specified exists and tries to build a ChangesFile (see
137-doc/nascentuploadfile.txt) object based on that. If anything goes
138-wrong during this process UploadError is raised:
139+NascentUpload Processing
140+------------------------
141+
142+Processing a NascentUpload consists of building files objects for each
143+specified file in the upload, executing all their specific checks and
144+collect all errors that may be generated. (see doc/nascentuploadfile.txt)
145+
146+First, NascentUpload verifies that the changes file specified exist, and
147+tries to build a ChangesFile (see doc/nascentuploadfile.txt) object based
148+on that.
149
150 >>> from lp.services.log.logger import DevNullLogger, FakeLogger
151- >>> NascentUpload.from_changesfile_path(
152+
153+ >>> nonexistent = NascentUpload.from_changesfile_path(
154 ... datadir("DOES-NOT-EXIST"), buildd_policy, FakeLogger())
155+ >>> nonexistent.process()
156 Traceback (most recent call last):
157 ...
158- UploadError:...
159-
160-Otherwise a ChangesFile object is ready to use.
161+ EarlyReturnUploadError: An error occurred that prevented further
162+ processing.
163+ >>> nonexistent.is_rejected
164+ True
165+ >>> print nonexistent.rejection_message
166+ Unable to read DOES-NOT-EXIST: ...
167
168 >>> quodlibet = NascentUpload.from_changesfile_path(
169 ... datadir("quodlibet_0.13.1-1_i386.changes"),
170 ... anything_policy, DevNullLogger())
171-
172- >>> quodlibet.changes
173- <lp.archiveuploader.changesfile.ChangesFile ...>
174-
175-
176-NascentUpload Processing
177-------------------------
178-
179-Processing a NascentUpload consists of building files objects for each
180-specified file in the upload, executing all their specific checks and
181-collect all errors that may be generated. (see doc/nascentuploadfile.txt)
182-
183 >>> quodlibet.process()
184 >>> for f in quodlibet.changes.files:
185 ... print f.filename, f
186
187=== modified file 'lib/lp/archiveuploader/tests/nascentuploadfile.txt'
188--- lib/lp/archiveuploader/tests/nascentuploadfile.txt 2017-08-03 14:26:40 +0000
189+++ lib/lp/archiveuploader/tests/nascentuploadfile.txt 2017-09-17 15:23:26 +0000
190@@ -72,10 +72,14 @@
191 >>> ed_binary_changes = ChangesFile(
192 ... datadir('ed_0.2-20_i386.changes.binary-only'),
193 ... modified_insecure_policy, DevNullLogger())
194+ >>> len(list(ed_binary_changes.parseChanges()))
195+ 0
196
197 >>> ed_source_changes = ChangesFile(
198 ... datadir('ed_0.2-20_source.changes'),
199 ... modified_insecure_policy, DevNullLogger())
200+ >>> len(list(ed_source_changes.parseChanges()))
201+ 0
202
203 Make sure we are not getting any exceptions due to a malformed changes
204 file name.
205
206=== modified file 'lib/lp/archiveuploader/tests/test_changesfile.py'
207--- lib/lp/archiveuploader/tests/test_changesfile.py 2017-03-29 09:28:09 +0000
208+++ lib/lp/archiveuploader/tests/test_changesfile.py 2017-09-17 15:23:26 +0000
209@@ -182,7 +182,10 @@
210 changes.dump(changes_fd)
211 finally:
212 changes_fd.close()
213- return ChangesFile(path, self.policy, self.logger)
214+ changesfile = ChangesFile(path, self.policy, self.logger)
215+ for error in changesfile.parseChanges():
216+ raise error
217+ return changesfile
218
219 def getBaseChanges(self):
220 contents = Changes()
221@@ -364,36 +367,39 @@
222 import_public_test_keys()
223
224 def test_valid_signature_accepted(self):
225- # A correctly signed changes file is excepted, and all its
226+ # A correctly signed changes file is accepted, and all its
227 # content is parsed.
228 path = datadir('signatures/signed.changes')
229- parsed = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
230+ changesfile = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
231+ self.assertEqual([], list(changesfile.parseChanges()))
232 self.assertEqual(
233 getUtility(IPersonSet).getByEmail('foo.bar@canonical.com'),
234- parsed.signer)
235+ changesfile.signer)
236 expected = "\AFormat: 1.7\n.*foo_1.0-1.diff.gz\Z"
237 self.assertTextMatchesExpressionIgnoreWhitespace(
238 expected,
239- parsed.parsed_content)
240+ changesfile.parsed_content)
241
242 def test_no_signature_rejected(self):
243 # An unsigned changes file is rejected.
244 path = datadir('signatures/unsigned.changes')
245- self.assertRaises(
246- UploadError,
247- ChangesFile, path, InsecureUploadPolicy(), BufferLogger())
248+ changesfile = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
249+ errors = list(changesfile.parseChanges())
250+ self.assertIsInstance(errors[0], UploadError)
251+ self.assertEqual(1, len(errors))
252
253 def test_prefix_ignored(self):
254 # A signed changes file with an unsigned prefix has only the
255 # signed part parsed.
256 path = datadir('signatures/prefixed.changes')
257- parsed = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
258+ changesfile = ChangesFile(path, InsecureUploadPolicy(), BufferLogger())
259+ self.assertEqual([], list(changesfile.parseChanges()))
260 self.assertEqual(
261 getUtility(IPersonSet).getByEmail('foo.bar@canonical.com'),
262- parsed.signer)
263+ changesfile.signer)
264 expected = "\AFormat: 1.7\n.*foo_1.0-1.diff.gz\Z"
265 self.assertTextMatchesExpressionIgnoreWhitespace(
266 expected,
267- parsed.parsed_content)
268- self.assertEqual("breezy", parsed.suite_name)
269- self.assertNotIn("evil", parsed.changes_comment)
270+ changesfile.parsed_content)
271+ self.assertEqual("breezy", changesfile.suite_name)
272+ self.assertNotIn("evil", changesfile.changes_comment)
273
274=== modified file 'lib/lp/archiveuploader/tests/test_nascentuploadfile.py'
275--- lib/lp/archiveuploader/tests/test_nascentuploadfile.py 2016-11-08 20:22:59 +0000
276+++ lib/lp/archiveuploader/tests/test_nascentuploadfile.py 2017-09-17 15:23:26 +0000
277@@ -208,7 +208,9 @@
278 path = os.path.join(tempdir, filename)
279 with open(path, "w") as changes_fd:
280 changes.dump(changes_fd)
281- return ChangesFile(path, self.policy, self.logger)
282+ changesfile = ChangesFile(path, self.policy, self.logger)
283+ self.assertEqual([], list(changesfile.parseChanges()))
284+ return changesfile
285
286
287 class DSCFileTests(PackageUploadFileTestCase):
288
289=== modified file 'lib/lp/archiveuploader/tests/test_sync_notification.py'
290--- lib/lp/archiveuploader/tests/test_sync_notification.py 2015-08-26 13:41:21 +0000
291+++ lib/lp/archiveuploader/tests/test_sync_notification.py 2017-09-17 15:23:26 +0000
292@@ -1,4 +1,4 @@
293-# Copyright 2012-2015 Canonical Ltd. This software is licensed under the
294+# Copyright 2012-2016 Canonical Ltd. This software is licensed under the
295 # GNU Affero General Public License version 3 (see the file LICENSE).
296
297 """Test notification behaviour for cross-distro package syncs."""
298@@ -55,6 +55,7 @@
299 self.raw_content = open(file_path).read()
300 self.signingkey = None
301
302+ parseChanges = FakeMethod([])
303 checkFileName = FakeMethod([])
304 processAddresses = FakeMethod([])
305 processFiles = FakeMethod([])
306
307=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
308--- lib/lp/archiveuploader/tests/test_uploadprocessor.py 2017-04-21 15:21:27 +0000
309+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py 2017-09-17 15:23:26 +0000
310@@ -51,6 +51,7 @@
311 IBuildFarmJobBehaviour,
312 )
313 from lp.registry.interfaces.distribution import IDistributionSet
314+from lp.registry.interfaces.gpg import IGPGKeySet
315 from lp.registry.interfaces.person import IPersonSet
316 from lp.registry.interfaces.pocket import PackagePublishingPocket
317 from lp.registry.interfaces.series import SeriesStatus
318@@ -240,7 +241,7 @@
319 excName = excClass.__name__
320 else:
321 excName = str(excClass)
322- raise self.failureException, "%s not raised" % excName
323+ raise self.failureException("%s not raised" % excName)
324
325 def setupBreezy(self, name="breezy", permitted_formats=None):
326 """Create a fresh distroseries in ubuntu.
327@@ -1955,10 +1956,47 @@
328
329 self.assertEqual(UploadStatusEnum.REJECTED, result)
330 self.assertLogContains(
331- "INFO Failed to parse changes file")
332+ "INFO Not sending rejection notice without a signing key.")
333 self.assertEmailQueueLength(0)
334 self.assertEqual([], self.oopses)
335
336+ def test_deactivated_key_upload_sends_mail(self):
337+ # An upload signed with a deactivated key does not OOPS and sends a
338+ # rejection email.
339+ self.switchToAdmin()
340+ fingerprint = "340CA3BB270E2716C9EE0B768E7EB7086C64A8C5"
341+ gpgkeyset = getUtility(IGPGKeySet)
342+ gpgkeyset.deactivate(gpgkeyset.getByFingerprint(fingerprint))
343+ self.switchToUploader()
344+
345+ uploadprocessor = self.setupBreezyAndGetUploadProcessor()
346+ upload_dir = self.queueUpload("netapplet_1.0-1-signed")
347+
348+ [result] = self.processUpload(uploadprocessor, upload_dir)
349+
350+ self.assertEqual(UploadStatusEnum.REJECTED, result)
351+ base_contents = [
352+ "Subject: [ubuntu] netapplet_1.0-1_source.changes (Rejected)",
353+ "File %s/netapplet_1.0-1-signed/netapplet_1.0-1_source.changes "
354+ "is signed with a deactivated key %s" % (
355+ self.incoming_folder, fingerprint),
356+ ]
357+ expected = []
358+ expected.append({
359+ "contents": base_contents + [
360+ "You are receiving this email because you are the most "
361+ "recent person",
362+ "listed in this package's changelog."],
363+ "recipient": "daniel.silverstone@canonical.com",
364+ })
365+ expected.append({
366+ "contents": base_contents + [
367+ "You are receiving this email because you made this upload."],
368+ "recipient": "foo.bar@canonical.com",
369+ })
370+ self.assertEmails(expected)
371+ self.assertEqual([], self.oopses)
372+
373 def test_ddeb_upload_overrides(self):
374 # DDEBs should always be overridden to the same values as their
375 # counterpart DEB's.
376
377=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
378--- lib/lp/archiveuploader/uploadprocessor.py 2017-01-10 15:18:48 +0000
379+++ lib/lp/archiveuploader/uploadprocessor.py 2017-09-17 15:23:26 +0000
380@@ -338,22 +338,8 @@
381 # The path we want for NascentUpload is the path to the folder
382 # containing the changes file (and the other files referenced by it).
383 changesfile_path = os.path.join(self.upload_path, changes_file)
384- try:
385- upload = NascentUpload.from_changesfile_path(
386- changesfile_path, policy, self.processor.log)
387- except UploadError as e:
388- # We failed to parse the changes file, so we have no key or
389- # Changed-By to notify of the rejection. Just log it and
390- # move on.
391- # XXX wgrant 2011-09-29 bug=499438: With some refactoring we
392- # could do better here: if we have a signature then we have
393- # somebody to email, even if the rest of the file is
394- # corrupt.
395- logger.info(
396- "Failed to parse changes file '%s': %s" % (
397- os.path.join(self.upload_path, changes_file),
398- str(e)))
399- return UploadStatusEnum.REJECTED
400+ upload = NascentUpload.from_changesfile_path(
401+ changesfile_path, policy, self.processor.log)
402
403 # Reject source upload to buildd upload paths.
404 first_path = relative_path.split(os.path.sep)[0]
405@@ -411,7 +397,16 @@
406 notify = False
407 if upload.is_rejected:
408 result = UploadStatusEnum.REJECTED
409- upload.do_reject(notify)
410+ if upload.changes.parsed_content is not None:
411+ # We got past the point of checking any required
412+ # signature, so we can do a proper rejection.
413+ upload.do_reject(notify)
414+ else:
415+ # The upload required a signature and either didn't have
416+ # one or we failed to verify it, so we have nobody to
417+ # notify. Just log it and move on.
418+ logger.info(
419+ "Not sending rejection notice without a signing key.")
420 self.processor.ztm.abort()
421 else:
422 successful = self._acceptUpload(upload, notify)
423
424=== modified file 'lib/lp/soyuz/doc/distroseriesqueue-translations.txt'
425--- lib/lp/soyuz/doc/distroseriesqueue-translations.txt 2015-09-04 12:19:07 +0000
426+++ lib/lp/soyuz/doc/distroseriesqueue-translations.txt 2017-09-17 15:23:26 +0000
427@@ -87,10 +87,9 @@
428 >>> pmount_upload = NascentUpload.from_changesfile_path(
429 ... datadir('pmount_0.9.7-2ubuntu2_amd64.changes'),
430 ... buildd_policy, FakeLogger())
431- DEBUG pmount_0.9.7-2ubuntu2_amd64.changes can be unsigned.
432-
433 >>> pmount_upload.process(build=build)
434 DEBUG Beginning processing.
435+ DEBUG pmount_0.9.7-2ubuntu2_amd64.changes can be unsigned.
436 DEBUG Verifying the changes file.
437 DEBUG Verifying files in upload.
438 DEBUG Verifying binary pmount_0.9.7-2ubuntu2_amd64.deb