launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-store-release-refresh into lp:launchpad.

Commit message:
Make the snap store client cope with a few more edge cases.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1766911 in Launchpad itself: "confusing message: Store upload failed: Authorization failed"
  https://bugs.launchpad.net/launchpad/+bug/1766911

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

=== modified file 'lib/lp/snappy/interfaces/snapstoreclient.py'
--- lib/lp/snappy/interfaces/snapstoreclient.py	2018-02-15 21:38:41 +0000
+++ lib/lp/snappy/interfaces/snapstoreclient.py	2018-05-03 00:42:12 +0000
@@ -1,4 +1,4 @@
-# Copyright 2016-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interface for communication with the snap store."""
@@ -116,6 +116,19 @@
         :param snap: An `ISnap` whose discharge macaroon needs to be refreshed.
         """
 
+    def refreshIfNecessary(snap, f, *args, **kwargs):
+        """Call a function, refreshing macaroons if necessary.
+
+        If the called function raises `NeedsRefreshResponse`, then this
+        calls `refreshDischargeMacaroon` and tries again.
+
+        :param snap: An `ISnap` whose discharge macaroon may need to be
+            refreshed.
+        :param f: The function to call.
+        :param args: Positional arguments to `f`.
+        :param kwargs: Keyword arguments to `f`.
+        """
+
     def checkStatus(status_url):
         """Poll the store once for upload scan status.
 

=== modified file 'lib/lp/snappy/model/snapstoreclient.py'
--- lib/lp/snappy/model/snapstoreclient.py	2018-02-19 17:50:36 +0000
+++ lib/lp/snappy/model/snapstoreclient.py	2018-05-03 00:42:12 +0000
@@ -1,4 +1,4 @@
-# Copyright 2016-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Communication with the snap store."""
@@ -104,9 +104,23 @@
                         self.logger.debug(
                             "%s macaroon: OpenID identifier: %s" %
                             (macaroon_name, account["openid"]))
+                elif key == "acl":
+                    self.logger.debug(
+                        "%s macaroon: permissions: %s" %
+                        (macaroon_name, value))
+                elif key == "channel":
+                    self.logger.debug(
+                        "%s macaroon: channels: %s" % (macaroon_name, value))
+                elif key == "expires":
+                    self.logger.debug(
+                        "%s macaroon: expires: %s" % (macaroon_name, value))
                 elif key == "package_id":
                     self.logger.debug(
                         "%s macaroon: snap-ids: %s" % (macaroon_name, value))
+                elif key == "valid_since":
+                    self.logger.debug(
+                        "%s macaroon: valid since: %s" %
+                        (macaroon_name, value))
             except ValueError:
                 pass
 
@@ -266,7 +280,8 @@
                         "Macaroon needs_refresh=1"):
                     raise NeedsRefreshResponse()
                 else:
-                    raise UnauthorizedUploadResponse("Authorization failed.")
+                    raise cls._makeSnapStoreError(
+                        UnauthorizedUploadResponse, e)
             raise cls._makeSnapStoreError(UploadFailedResponse, e)
 
     @classmethod
@@ -277,13 +292,8 @@
             if not lfa.filename.endswith(".snap"):
                 continue
             upload_data = cls._uploadFile(lfa, lfc)
-            try:
-                return cls._uploadApp(snapbuild.snap, upload_data)
-            except NeedsRefreshResponse:
-                # Try to automatically refresh the discharge macaroon and
-                # retry the upload.
-                cls.refreshDischargeMacaroon(snapbuild.snap)
-                return cls._uploadApp(snapbuild.snap, upload_data)
+            return cls.refreshIfNecessary(
+                snapbuild.snap, cls._uploadApp, snapbuild.snap, upload_data)
 
     @classmethod
     def refreshDischargeMacaroon(cls, snap):
@@ -306,6 +316,15 @@
             raise cls._makeSnapStoreError(BadRefreshResponse, e)
 
     @classmethod
+    def refreshIfNecessary(cls, snap, f, *args, **kwargs):
+        """See `ISnapStoreClient`."""
+        try:
+            return f(*args, **kwargs)
+        except NeedsRefreshResponse:
+            cls.refreshDischargeMacaroon(snap)
+            return f(*args, **kwargs)
+
+    @classmethod
     def checkStatus(cls, status_url):
         """See `ISnapStoreClient`."""
         try:
@@ -374,13 +393,8 @@
         return channels
 
     @classmethod
-    def release(cls, snapbuild, revision):
-        """See `ISnapStoreClient`."""
-        assert config.snappy.store_url is not None
-        snap = snapbuild.snap
-        assert snap.store_name is not None
-        assert snap.store_series is not None
-        assert snap.store_channels
+    def _release(cls, snap, revision):
+        """Release a snap revision to specified channels."""
         release_url = urlappend(
             config.snappy.store_url, "dev/api/snap-release/")
         data = {
@@ -400,4 +414,18 @@
                     snap.store_secrets["root"],
                     snap.store_secrets.get("discharge")))
         except requests.HTTPError as e:
+            if e.response.status_code == 401:
+                if (e.response.headers.get("WWW-Authenticate") ==
+                        "Macaroon needs_refresh=1"):
+                    raise NeedsRefreshResponse()
             raise cls._makeSnapStoreError(ReleaseFailedResponse, e)
+
+    @classmethod
+    def release(cls, snapbuild, revision):
+        """See `ISnapStoreClient`."""
+        assert config.snappy.store_url is not None
+        snap = snapbuild.snap
+        assert snap.store_name is not None
+        assert snap.store_series is not None
+        assert snap.store_channels
+        cls.refreshIfNecessary(snap, cls._release, snap, revision)

=== modified file 'lib/lp/snappy/tests/test_snapstoreclient.py'
--- lib/lp/snappy/tests/test_snapstoreclient.py	2018-02-15 21:38:41 +0000
+++ lib/lp/snappy/tests/test_snapstoreclient.py	2018-05-03 00:42:12 +0000
@@ -1,4 +1,4 @@
-# Copyright 2016-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for communication with the snap store."""
@@ -452,6 +452,12 @@
             return {
                 "status_code": 401,
                 "headers": {"WWW-Authenticate": 'Macaroon realm="Devportal"'},
+                "content": {
+                    "error_list": [{
+                        "code": "macaroon-permission-required",
+                        "message": "Permission is required: package_push",
+                        }],
+                    },
                 }
 
         store_secrets = self._make_store_secrets()
@@ -460,8 +466,10 @@
         with dbuser(config.ISnapStoreUploadJobSource.dbuser):
             with HTTMock(self._unscanned_upload_handler, snap_push_handler,
                          self._macaroon_refresh_handler):
-                self.assertRaises(
-                    UnauthorizedUploadResponse, self.client.upload, snapbuild)
+                self.assertRaisesWithContent(
+                    UnauthorizedUploadResponse,
+                    "Permission is required: package_push",
+                    self.client.upload, snapbuild)
 
     def test_upload_needs_discharge_macaroon_refresh(self):
         @urlmatch(path=r".*/snap-push/$")
@@ -734,6 +742,34 @@
                 "channels": ["stable", "edge"], "series": "rolling",
                 }))
 
+    def test_release_needs_discharge_macaroon_refresh(self):
+        @urlmatch(path=r".*/snap-release/$")
+        def snap_release_handler(url, request):
+            snap_release_handler.call_count += 1
+            if snap_release_handler.call_count == 1:
+                self.first_snap_release_request = request
+                return {
+                    "status_code": 401,
+                    "headers": {
+                        "WWW-Authenticate": "Macaroon needs_refresh=1"}}
+            else:
+                return self._snap_release_handler(url, request)
+        snap_release_handler.call_count = 0
+
+        store_secrets = self._make_store_secrets()
+        with HTTMock(self._channels_handler):
+            snap = self.factory.makeSnap(
+                store_upload=True,
+                store_series=self.factory.makeSnappySeries(name="rolling"),
+                store_name="test-snap", store_secrets=store_secrets,
+                store_channels=["stable", "edge"])
+        snapbuild = self.factory.makeSnapBuild(snap=snap)
+        with HTTMock(snap_release_handler, self._macaroon_refresh_handler):
+            self.client.release(snapbuild, 1)
+        self.assertEqual(2, snap_release_handler.call_count)
+        self.assertNotEqual(
+            store_secrets["discharge"], snap.store_secrets["discharge"])
+
     def test_release_error(self):
         @urlmatch(path=r".*/snap-release/$")
         def handler(url, request):