launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> === 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
> @@ -45,3 +55,9 @@
>  
>          :param snapbuild: The `ISnapBuild` to upload.
>          """
> +
> +    def refresh_discharge_macaroon(snap):

Launchpad style for method names (though not function names) is namesLikeThis, so please make this "refreshDischargeMacaroon".

> +        """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
> @@ -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()

This needs clearer indentation.  I think just indenting the line with "Macaroon needs_refresh=1" on it by one level would help.

>              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)

SSO requires this to be Content-Type: application/json, so you'll need to use json=data here rather than data=data.  Compare https://code.launchpad.net/~cjwatson/launchpad/snap-upload-content-type/+merge/295983.

> +            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])


-- 
https://code.launchpad.net/~maxiberta/launchpad/snap-refresh-macaroon/+merge/295881
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.