launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-bad-refresh-response-mail into lp:launchpad with lp:~cjwatson/launchpad/snap-better-upload-failures as a prerequisite.

Commit message:
Send mail rather than OOPSing if refreshing snap store upload macaroons fails.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1668368 in Launchpad itself: "403 from SSO/api/v2/tokens/refresh results in OOPS rather than helpful email"
  https://bugs.launchpad.net/launchpad/+bug/1668368

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-bad-refresh-response-mail/+merge/325508
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-bad-refresh-response-mail into lp:launchpad.

=== added file 'lib/lp/snappy/emailtemplates/snapbuild-refreshfailed.txt'
--- lib/lp/snappy/emailtemplates/snapbuild-refreshfailed.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/snappy/emailtemplates/snapbuild-refreshfailed.txt	2017-06-12 16:48:30 +0000
@@ -0,0 +1,10 @@
+Launchpad failed to refresh the authorization tokens used to upload this
+snap package to the store:
+
+  %(store_error_message)s
+
+Depending on the error message above, this may indicate a bug.  If so, you
+may be able to work around it by reauthorizing Launchpad to upload the
+package in question:
+
+  %(snap_authorize_url)s

=== modified file 'lib/lp/snappy/interfaces/snapstoreclient.py'
--- lib/lp/snappy/interfaces/snapstoreclient.py	2017-06-12 16:48:30 +0000
+++ lib/lp/snappy/interfaces/snapstoreclient.py	2017-06-12 16:48:30 +0000
@@ -98,6 +98,11 @@
 
         :param snapbuild: The `ISnapBuild` to upload.
         :return: A URL to poll for upload processing status.
+        :raises BadRefreshResponse: if the authorising macaroons need to be
+            refreshed, but attempting to do so fails.
+        :raises UnauthorizedUploadResponse: if the user who authorised this
+            upload is not themselves authorised to upload the snap in
+            question.
         :raises UploadFailedResponse: if uploading the build to the store
             failed.
         """

=== modified file 'lib/lp/snappy/mail/snapbuild.py'
--- lib/lp/snappy/mail/snapbuild.py	2017-02-27 18:46:38 +0000
+++ lib/lp/snappy/mail/snapbuild.py	2017-06-12 16:48:30 +0000
@@ -47,6 +47,20 @@
             "snap-build-upload-unauthorized", build)
 
     @classmethod
+    def forRefreshFailure(cls, build):
+        """Create a mailer for notifying about macaroon refresh failures.
+
+        :param build: The relevant build.
+        """
+        requester = build.requester
+        recipients = {requester: RecipientReason.forBuildRequester(requester)}
+        return cls(
+            "Refreshing store authorization failed for %(snap_name)s",
+            "snapbuild-refreshfailed.txt", recipients,
+            config.canonical.noreply_from_address,
+            "snap-build-upload-refresh-failed", build)
+
+    @classmethod
     def forUploadFailure(cls, build):
         """Create a mailer for notifying about store upload failures.
 

=== modified file 'lib/lp/snappy/model/snapbuildjob.py'
--- lib/lp/snappy/model/snapbuildjob.py	2017-06-12 16:48:30 +0000
+++ lib/lp/snappy/model/snapbuildjob.py	2017-06-12 16:48:30 +0000
@@ -55,6 +55,7 @@
     ISnapStoreUploadJobSource,
     )
 from lp.snappy.interfaces.snapstoreclient import (
+    BadRefreshResponse,
     BadScanStatusResponse,
     ISnapStoreClient,
     ReleaseFailedResponse,
@@ -305,6 +306,9 @@
             if isinstance(e, UnauthorizedUploadResponse):
                 mailer = SnapBuildMailer.forUnauthorizedUpload(self.snapbuild)
                 mailer.sendAll()
+            elif isinstance(e, BadRefreshResponse):
+                mailer = SnapBuildMailer.forRefreshFailure(self.snapbuild)
+                mailer.sendAll()
             elif isinstance(e, UploadFailedResponse):
                 mailer = SnapBuildMailer.forUploadFailure(self.snapbuild)
                 mailer.sendAll()

=== modified file 'lib/lp/snappy/tests/test_snapbuildjob.py'
--- lib/lp/snappy/tests/test_snapbuildjob.py	2017-06-12 16:48:30 +0000
+++ lib/lp/snappy/tests/test_snapbuildjob.py	2017-06-12 16:48:30 +0000
@@ -28,6 +28,7 @@
     ISnapStoreUploadJob,
     )
 from lp.snappy.interfaces.snapstoreclient import (
+    BadRefreshResponse,
     BadScanStatusResponse,
     ISnapStoreClient,
     ReleaseFailedResponse,
@@ -230,6 +231,56 @@
         self.assertWebhookDeliveries(
             snapbuild, ["Pending", "Failed to upload"])
 
+    def test_run_refresh_failure_notifies(self):
+        # A run that gets a failure when trying to refresh macaroons sends
+        # mail.
+        requester = self.factory.makePerson(name="requester")
+        requester_team = self.factory.makeTeam(
+            owner=requester, name="requester-team", members=[requester])
+        snapbuild = self.makeSnapBuild(
+            requester=requester_team, name="test-snap", owner=requester_team)
+        self.assertContentEqual([], snapbuild.store_upload_jobs)
+        job = SnapStoreUploadJob.create(snapbuild)
+        client = FakeSnapStoreClient()
+        client.upload.failure = BadRefreshResponse("SSO melted.")
+        self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
+        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+            JobRunner([job]).runAll()
+        self.assertEqual([((snapbuild,), {})], client.upload.calls)
+        self.assertEqual([], client.checkStatus.calls)
+        self.assertEqual([], client.release.calls)
+        self.assertContentEqual([job], snapbuild.store_upload_jobs)
+        self.assertIsNone(job.store_url)
+        self.assertIsNone(job.store_revision)
+        self.assertEqual("SSO melted.", job.error_message)
+        [notification] = pop_notifications()
+        self.assertEqual(
+            config.canonical.noreply_from_address, notification["From"])
+        self.assertEqual(
+            "Requester <%s>" % requester.preferredemail.email,
+            notification["To"])
+        subject = notification["Subject"].replace("\n ", " ")
+        self.assertEqual(
+            "Refreshing store authorization failed for test-snap", subject)
+        self.assertEqual(
+            "Requester @requester-team",
+            notification["X-Launchpad-Message-Rationale"])
+        self.assertEqual(
+            requester_team.name, notification["X-Launchpad-Message-For"])
+        self.assertEqual(
+            "snap-build-upload-refresh-failed",
+            notification["X-Launchpad-Notification-Type"])
+        body, footer = notification.get_payload(decode=True).split("\n-- \n")
+        self.assertIn(
+            "http://launchpad.dev/~requester-team/+snap/test-snap/+authorize";,
+            body)
+        self.assertEqual(
+            "http://launchpad.dev/~requester-team/+snap/test-snap/+build/%d\n";
+            "Your team Requester Team is the requester of the build.\n" %
+            snapbuild.id, footer)
+        self.assertWebhookDeliveries(
+            snapbuild, ["Pending", "Failed to upload"])
+
     def test_run_upload_failure_notifies(self):
         # A run that gets some other upload failure from the store sends
         # mail.