launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/snappy/browser/snap.py'
> --- lib/lp/snappy/browser/snap.py	2016-04-19 10:15:38 +0000
> +++ lib/lp/snappy/browser/snap.py	2016-05-12 15:40:52 +0000
> @@ -503,6 +515,111 @@
>                          data['processors'].append(processor)
>  
>  
> +class SnapAuthorizationException(Exception):
> +    pass
> +
> +
> +class SnapAuthorizeView(LaunchpadEditFormView):
> +    """View for authorizing snap package uploads to the store."""
> +
> +    class schema(Interface):
> +        """Schema for authorizing snap package uploads to the store."""
> +
> +        callback = Bool()
> +        discharge_macaroon = TextLine(
> +            title=u'Serialized discharge macaroon', required=True)
> +
> +    render_context = False
> +
> +    @staticmethod
> +    def extractSSOCaveat(macaroon):
> +        locations = [
> +            urlsplit(root).netloc
> +            for root in CurrentOpenIDEndPoint.getAllRootURLs()]
> +        sso_caveats = [
> +            c for c in macaroon.third_party_caveats()
> +            if c.location in locations]
> +        # We must have exactly one SSO caveat; more than one should never be
> +        # required and could be an attempt to substitute weaker caveats.  We
> +        # might as well OOPS here, even though the cause of this is probably
> +        # in some other service, since the user can't do anything about it
> +        # and it should show up in our OOPS reports.
> +        if not sso_caveats:
> +            raise SnapAuthorizationException(
> +                "Macaroon '%s' has no SSO caveats" % macaroon.serialize())
> +        elif len(sso_caveats) > 1:
> +            raise SnapAuthorizationException(
> +                "Macaroon '%s' has multiple SSO caveats" %
> +                macaroon.serialize())

The root macaroon itself may in future convey authority, so I'm not sure about including the whole thing in an exception message.

> +        return sso_caveats[0]
> +
> +    @classmethod
> +    def requestAuthorization(cls, snap, request):
> +        """Begin the process of authorizing uploads of a snap package."""
> +        if snap.store_series is None:
> +            request.response.addInfoNotification(
> +                _(u'Cannot authorize uploads of a snap package with no '
> +                  u'store series.'))
> +            request.response.redirect(canonical_url(snap))
> +            return
> +        if snap.store_name is None:
> +            request.response.addInfoNotification(
> +                _(u'Cannot authorize uploads of a snap package with no '
> +                  u'store name.'))
> +            request.response.redirect(canonical_url(snap))
> +            return
> +        snap_store_client = getUtility(ISnapStoreClient)
> +        root_macaroon_raw = snap_store_client.requestPackageUploadPermission(
> +            snap.store_series, snap.store_name)
> +        sso_caveat = cls.extractSSOCaveat(
> +            Macaroon.deserialize(root_macaroon_raw))
> +        snap.store_secrets = {'root': root_macaroon_raw}
> +        transaction.commit()
> +        base_url = canonical_url(snap, view_name='+authorize')
> +        login_url = urlappend(base_url, '+login')
> +        login_url += '?%s' % urlencode([
> +            ('field.callback', 'on'),
> +            ('macaroon_caveat_id', sso_caveat.caveat_id),
> +            ('discharge_macaroon_field', 'field.discharge_macaroon'),
> +            ])
> +        return login_url
> +
> +    def render(self):
> +        data = {}
> +        self.validate_widgets(data)
> +        if not data.get('callback') or self.context.store_secrets is None:
> +            login_url = self.requestAuthorization(self.context, self.request)
> +            if login_url is not None:
> +                self.request.response.redirect(login_url)
> +            return
> +        if not data.get('discharge_macaroon'):
> +            self.request.response.addInfoNotification(structured(
> +                _(u'Uploads of %(snap)s to the store were not authorized.'),
> +                snap=self.context.name))
> +            self.request.response.redirect(canonical_url(self.context))
> +            return
> +        # We have to set a whole new dict here to avoid problems with
> +        # security proxies.
> +        new_store_secrets = dict(self.context.store_secrets)
> +        new_store_secrets['discharge'] = data['discharge_macaroon']
> +        self.context.store_secrets = new_store_secrets
> +        # XXX cjwatson 2016-04-18: This may be a GET request due to coming
> +        # from a redirection at the end of the OpenID exchange, but we need
> +        # to store the macaroons.  Perhaps we should return an
> +        # auto-submitting form instead?  The operation performed by this
> +        # view is at least idempotent.
> +        transaction.commit()

This won't work reliably, as a GET may be running on a read-only database replica.

> +        self.request.response.addInfoNotification(structured(
> +            _(u'Uploads of %(snap)s to the store are now authorized.'),
> +            snap=self.context.name))
> +        self.request.response.redirect(canonical_url(self.context))
> +
> +    @property
> +    def adapters(self):
> +        """See `LaunchpadFormView`."""
> +        return {self.schema: self.context}
> +
> +
>  class SnapDeleteView(BaseSnapEditView):
>      """View for deleting snap packages."""
>  


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-authorize-view/+merge/294358
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.