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):
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : launchpad-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
