launchpad-reviewers team mailing list archive

[Maximiliano Bertacchini <maximiliano.bertacchini@xxxxxxxxxxxxx>]

Maximiliano Bertacchini has proposed merging lp:~maxiberta/launchpad/snap-refresh-macaroon into lp:launchpad.

Commit message:
Refresh discharge macaroon (if needed) on snap upload.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~maxiberta/launchpad/snap-refresh-macaroon/+merge/295881

Refresh discharge macaroon (if needed) on snap upload.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~maxiberta/launchpad/snap-refresh-macaroon into lp:launchpad.

=== modified file 'lib/lp/snappy/interfaces/snapstoreclient.py'
--- lib/lp/snappy/interfaces/snapstoreclient.py	2016-05-09 16:03:18 +0000
+++ lib/lp/snappy/interfaces/snapstoreclient.py	2016-05-26 20:23:53 +0000
@@ -7,9 +7,11 @@
 
 __metaclass__ = type
 __all__ = [
+    'BadRefreshResponse',
     'BadRequestPackageUploadResponse',
     'BadUploadResponse',
     'ISnapStoreClient',
+    'NeedsRefreshResponse',
     ]
 
 from zope.interface import Interface
@@ -23,6 +25,14 @@
     pass
 
 
+class BadRefreshResponse(Exception):
+    pass
+
+
+class NeedsRefreshResponse(Exception):
+    pass
+
+
 class ISnapStoreClient(Interface):
     """Interface for the API provided by the snap store."""
 
@@ -45,3 +55,9 @@
 
         :param snapbuild: The `ISnapBuild` to upload.
         """
+
+    def refresh_discharge_macaroon(snap):
+        """Refresh a snap's discharge macaroon.
+
+        :param snap: An `ISnap` whose discharge macaroon needs to be refreshed.
+        """

=== modified file 'lib/lp/snappy/model/snapstoreclient.py'
--- lib/lp/snappy/model/snapstoreclient.py	2016-05-24 09:36:45 +0000
+++ lib/lp/snappy/model/snapstoreclient.py	2016-05-26 20:23:53 +0000
@@ -23,9 +23,11 @@
 from lp.services.timeout import urlfetch
 from lp.services.webapp.url import urlappend
 from lp.snappy.interfaces.snapstoreclient import (
+    BadRefreshResponse,
     BadRequestPackageUploadResponse,
     BadUploadResponse,
     ISnapStoreClient,
+    NeedsRefreshResponse,
     )
 
 
@@ -151,7 +153,6 @@
             "updown_id": upload_data["upload_id"],
             "series": snap.store_series.name,
             }
-        # XXX cjwatson 2016-04-20: handle refresh
         # XXX cjwatson 2016-05-09: This should add timeline information, but
         # that's currently difficult in jobs.
         try:
@@ -162,6 +163,10 @@
                     snap.store_secrets["root"],
                     snap.store_secrets["discharge"]))
         except requests.HTTPError as e:
+            if (e.response.status_code == 401 and
+                e.response.headers.get("WWW-Authenticate") ==
+                "Macaroon needs_refresh=1"):
+                raise NeedsRefreshResponse()
             raise BadUploadResponse(e.args[0])
 
     def upload(self, snapbuild):
@@ -169,4 +174,29 @@
         assert snapbuild.snap.can_upload_to_store
         for _, lfa, lfc in snapbuild.getFiles():
             upload_data = self._uploadFile(lfa, lfc)
-            self._uploadApp(snapbuild.snap, upload_data)
+            try:
+                self._uploadApp(snapbuild.snap, upload_data)
+            except NeedsRefreshResponse:
+                # Try to automatically refresh the discharge macaroon and
+                # retry the upload.
+                self.refresh_discharge_macaroon(snapbuild.snap)
+                self._uploadApp(snapbuild.snap, upload_data)
+
+    @classmethod
+    def refresh_discharge_macaroon(cls, snap):
+        assert config.launchpad.openid_provider_root is not None
+        assert snap.store_secrets is not None
+        refresh_url = urlappend(
+            config.launchpad.openid_provider_root, "api/v2/tokens/refresh")
+        data = {"discharge_macaroon": snap.store_secrets["discharge"]}
+        try:
+            response = urlfetch(refresh_url, method="POST", data=data)
+            response_data = response.json()
+            if "discharge_macaroon" not in response_data:
+                raise BadRefreshResponse(response.text)
+            # Set a new dict here to avoid problems with security proxies.
+            new_secrets = dict(snap.store_secrets)
+            new_secrets["discharge"] = response_data["discharge_macaroon"]
+            snap.store_secrets = new_secrets
+        except requests.HTTPError as e:
+            raise BadRefreshResponse(e.args[0])

=== modified file 'lib/lp/snappy/tests/test_snapstoreclient.py'
--- lib/lp/snappy/tests/test_snapstoreclient.py	2016-05-24 09:36:45 +0000
+++ lib/lp/snappy/tests/test_snapstoreclient.py	2016-05-26 20:23:53 +0000
@@ -153,8 +153,52 @@
         self.pushConfig(
             "snappy", store_url="http://sca.example/";,
             store_upload_url="http://updown.example/";)
+        self.pushConfig(
+            "launchpad", openid_provider_root="http://sso.example/";)
         self.client = getUtility(ISnapStoreClient)
 
+    def _make_store_secrets(self):
+        self.root_key = hashlib.sha256(
+            self.factory.getUniqueString()).hexdigest()
+        root_macaroon = Macaroon(key=self.root_key)
+        self.discharge_key = hashlib.sha256(
+            self.factory.getUniqueString()).hexdigest()
+        self.discharge_caveat_id = self.factory.getUniqueString()
+        root_macaroon.add_third_party_caveat(
+            "sso.example", self.discharge_key, self.discharge_caveat_id)
+        unbound_discharge_macaroon = Macaroon(
+            location="sso.example", key=self.discharge_key,
+            identifier=self.discharge_caveat_id)
+        return {
+            "root": root_macaroon.serialize(),
+            "discharge": unbound_discharge_macaroon.serialize(),
+            }
+
+    @urlmatch(path=r".*/unscanned-upload/$")
+    def _unscanned_upload_handler(self, url, request):
+        self.unscanned_upload_request = request
+        return {
+            "status_code": 200,
+            "content": {"successful": True, "upload_id": 1},
+            }
+
+    @urlmatch(path=r".*/snap-upload/$")
+    def _snap_upload_handler(self, url, request):
+        self.snap_upload_request = request
+        return {"status_code": 202, "content": {"success": True}}
+
+    @urlmatch(path=r".*/api/v2/tokens/refresh$")
+    def _macaroon_refresh_handler(self, url, request):
+        self.refresh_request = request
+        new_macaroon = Macaroon(
+            location="sso.example", key=self.discharge_key,
+            identifier=self.discharge_caveat_id)
+        new_macaroon.add_first_party_caveat("sso|expires|tomorrow")
+        return {
+            "status_code": 200,
+            "content": {"discharge_macaroon": new_macaroon.serialize()},
+            }
+
     def test_requestPackageUploadPermission(self):
         @all_requests
         def handler(url, request):
@@ -206,42 +250,16 @@
                 snappy_series, "test-snap")
 
     def test_upload(self):
-        @urlmatch(path=r".*/unscanned-upload/$")
-        def unscanned_upload_handler(url, request):
-            self.unscanned_upload_request = request
-            return {
-                "status_code": 200,
-                "content": {"successful": True, "upload_id": 1},
-                }
-
-        @urlmatch(path=r".*/snap-upload/$")
-        def snap_upload_handler(url, request):
-            self.snap_upload_request = request
-            return {"status_code": 202, "content": {"success": True}}
-
-        root_key = hashlib.sha256(self.factory.getUniqueString()).hexdigest()
-        root_macaroon = Macaroon(key=root_key)
-        discharge_key = hashlib.sha256(
-            self.factory.getUniqueString()).hexdigest()
-        discharge_caveat_id = self.factory.getUniqueString()
-        root_macaroon.add_third_party_caveat(
-            "sso.example", discharge_key, discharge_caveat_id)
-        unbound_discharge_macaroon = Macaroon(
-            location="sso.example", key=discharge_key,
-            identifier=discharge_caveat_id)
-        store_secrets = {
-            "root": root_macaroon.serialize(),
-            "discharge": unbound_discharge_macaroon.serialize(),
-            }
         snap = self.factory.makeSnap(
             store_upload=True,
             store_series=self.factory.makeSnappySeries(name="rolling"),
-            store_name="test-snap", store_secrets=store_secrets)
+            store_name="test-snap", store_secrets=self._make_store_secrets())
         snapbuild = self.factory.makeSnapBuild(snap=snap)
         lfa = self.factory.makeLibraryFileAlias(content="dummy snap content")
         self.factory.makeSnapFile(snapbuild=snapbuild, libraryfile=lfa)
         transaction.commit()
-        with HTTMock(unscanned_upload_handler, snap_upload_handler):
+        with HTTMock(
+            self._unscanned_upload_handler, self._snap_upload_handler):
             self.client.upload(snapbuild)
         self.assertThat(self.unscanned_upload_request, RequestMatches(
             url=Equals("http://updown.example/unscanned-upload/";),
@@ -255,7 +273,7 @@
         self.assertThat(self.snap_upload_request, RequestMatches(
             url=Equals("http://sca.example/dev/api/snap-upload/";),
             method=Equals("POST"),
-            auth=("Macaroon", MacaroonsVerify(root_key)),
+            auth=("Macaroon", MacaroonsVerify(self.root_key)),
             form_data={
                 "name": MatchesStructure.byEquality(
                     name="name", value="test-snap"),
@@ -263,3 +281,53 @@
                     name="updown_id", value="1"),
                 "series": MatchesStructure.byEquality(
                     name="series", value="rolling")}))
+
+    def test_upload_needs_discharge_macaroon_refresh(self):
+        @urlmatch(path=r".*/snap-upload/$")
+        def snap_upload_handler(url, request):
+            snap_upload_handler.call_count += 1
+            if snap_upload_handler.call_count == 1:
+                self.first_snap_upload_request = request
+                return {
+                    "status_code": 401,
+                    "headers": {
+                        "WWW-Authenticate": "Macaroon needs_refresh=1"}}
+            else:
+                return self._snap_upload_handler(url, request)
+        snap_upload_handler.call_count = 0
+
+        store_secrets = self._make_store_secrets()
+        snap = self.factory.makeSnap(
+            store_upload=True,
+            store_series=self.factory.makeSnappySeries(name="rolling"),
+            store_name="test-snap", store_secrets=store_secrets)
+        snapbuild = self.factory.makeSnapBuild(snap=snap)
+        lfa = self.factory.makeLibraryFileAlias(content="dummy snap content")
+        self.factory.makeSnapFile(snapbuild=snapbuild, libraryfile=lfa)
+        transaction.commit()
+
+        with HTTMock(self._unscanned_upload_handler, snap_upload_handler,
+                     self._macaroon_refresh_handler):
+            self.client.upload(snapbuild)
+        self.assertEqual(2, snap_upload_handler.call_count)
+        self.assertNotEqual(
+            store_secrets["discharge"], snap.store_secrets["discharge"])
+
+    def test_refresh_discharge_macaroon(self):
+        store_secrets = self._make_store_secrets()
+        snap = self.factory.makeSnap(
+            store_upload=True,
+            store_series=self.factory.makeSnappySeries(name="rolling"),
+            store_name="test-snap", store_secrets=store_secrets)
+
+        with HTTMock(self._macaroon_refresh_handler):
+            self.client.refresh_discharge_macaroon(snap)
+        self.assertThat(self.refresh_request, RequestMatches(
+            url=Equals("http://sso.example/api/v2/tokens/refresh";),
+            method=Equals("POST"),
+            form_data={
+                "discharge_macaroon": MatchesStructure.byEquality(
+                    name="discharge_macaroon",
+                    value=store_secrets["discharge"])}))
+        self.assertNotEqual(
+            store_secrets["discharge"], snap.store_secrets["discharge"])