launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #20345
[Merge] lp:~cjwatson/launchpad/snap-authorize-view into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-authorize-view into lp:launchpad with lp:~cjwatson/launchpad/login-discharge-macaroon as a prerequisite.
Commit message:
Add a Snap:+authorize view allowing a user to (re)authorise snap package uploads to the store.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1572605 in Launchpad itself: "Automatically upload snap builds to store"
https://bugs.launchpad.net/launchpad/+bug/1572605
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-authorize-view/+merge/294358
Now that +login supports acquiring discharge macaroons (see the prerequisite branch), we can add a view that fetches a root macaroon from SCA, sends the user off to SSO to get a discharge for it via OpenID, and stores the result when they come back. In subsequent branches, we'll redirect to this view when users make changes to store upload settings, and mail the user if their existing store secrets have expired pointing them to this view.
There are a couple of slightly shonky bits here, which I think are acceptable for the time being but are worth noting:
* +login redirects to this view at the end of the OpenID exchange, so we end up storing data in GET requests and manually committing transactions. It's at least idempotent, but perhaps in future we should do an OpenID-like thing where we return an auto-submitting form rather than redirecting.
* Since we don't want to give SSO access to the root macaroon (it happens to be fine in this instance, but is a poor precedent to set), we store it in Snap.store_secrets before the exchange is complete. This means that if you hit Snap:+authorize when you already had valid secrets then Launchpad won't be able to upload builds for you until you complete the exchange. Fortunately this is mostly "don't do that, then".
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-authorize-view into lp:launchpad.
=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py 2016-04-26 11:06:17 +0000
+++ lib/lp/registry/model/person.py 2016-05-11 11:45:30 +0000
@@ -289,6 +289,7 @@
OAuthAccessToken,
OAuthRequestToken,
)
+from lp.services.openid.adapters.openid import CurrentOpenIDEndPoint
from lp.services.openid.model.openididentifier import OpenIdIdentifier
from lp.services.propertycache import (
cachedproperty,
@@ -3280,13 +3281,7 @@
# + is reserved, so is not allowed to be reencoded in transit, so
# should never appear as its percent-encoded equivalent.
identifier_suffix = None
- roots = [config.launchpad.openid_provider_root]
- if config.launchpad.openid_alternate_provider_roots:
- roots.extend(
- [root.strip() for root in
- config.launchpad.openid_alternate_provider_roots.split(',')
- if root.strip()])
- for root in roots:
+ for root in CurrentOpenIDEndPoint.getAllRootURLs():
base = '%s+id/' % root
if identifier.startswith(base):
identifier_suffix = identifier.replace(base, '', 1)
=== modified file 'lib/lp/services/openid/adapters/openid.py'
--- lib/lp/services/openid/adapters/openid.py 2015-07-09 12:18:51 +0000
+++ lib/lp/services/openid/adapters/openid.py 2016-05-11 11:45:30 +0000
@@ -24,11 +24,21 @@
class CurrentOpenIDEndPoint:
"""A utility for working with multiple OpenID End Points."""
- @classmethod
- def getServiceURL(cls):
+ @staticmethod
+ def getServiceURL():
"""The OpenID server URL (/+openid) for the current request."""
return config.launchpad.openid_provider_root + '+openid'
+ @staticmethod
+ def getAllRootURLs():
+ """All configured OpenID provider root URLs."""
+ yield config.launchpad.openid_provider_root
+ alternate_roots = config.launchpad.openid_alternate_provider_roots
+ if alternate_roots:
+ for root in [r.strip() for r in alternate_roots.split(',')]:
+ if root:
+ yield root
+
@adapter(IAccount)
@implementer(IOpenIDPersistentIdentity)
=== modified file 'lib/lp/snappy/browser/configure.zcml'
--- lib/lp/snappy/browser/configure.zcml 2016-05-06 11:54:18 +0000
+++ lib/lp/snappy/browser/configure.zcml 2016-05-11 11:45:30 +0000
@@ -68,6 +68,11 @@
template="../templates/snap-edit.pt" />
<browser:page
for="lp.snappy.interfaces.snap.ISnap"
+ class="lp.snappy.browser.snap.SnapAuthorizeView"
+ permission="launchpad.Edit"
+ name="+authorize" />
+ <browser:page
+ for="lp.snappy.interfaces.snap.ISnap"
class="lp.snappy.browser.snap.SnapDeleteView"
permission="launchpad.Edit"
name="+delete"
=== 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-11 11:45:30 +0000
@@ -6,6 +6,7 @@
__metaclass__ = type
__all__ = [
'SnapAddView',
+ 'SnapAuthorizeView',
'SnapContextMenu',
'SnapDeleteView',
'SnapEditView',
@@ -15,18 +16,26 @@
'SnapView',
]
+from urllib import urlencode
+from urlparse import urlsplit
+
from lazr.restful.fields import Reference
from lazr.restful.interface import (
copy_field,
use_template,
)
+from pymacaroons import Macaroon
+import transaction
from zope.component import getUtility
from zope.interface import Interface
from zope.schema import (
+ Bool,
Choice,
List,
+ TextLine,
)
+from lp import _
from lp.app.browser.launchpadform import (
action,
custom_widget,
@@ -49,6 +58,7 @@
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.services.features import getFeatureFlag
from lp.services.helpers import english_list
+from lp.services.openid.adapters.openid import CurrentOpenIDEndPoint
from lp.services.webapp import (
canonical_url,
ContextMenu,
@@ -59,6 +69,7 @@
NavigationMenu,
stepthrough,
structured,
+ urlappend,
)
from lp.services.webapp.authorization import check_permission
from lp.services.webapp.breadcrumb import (
@@ -78,6 +89,7 @@
SnapPrivateFeatureDisabled,
)
from lp.snappy.interfaces.snapbuild import ISnapBuildSet
+from lp.snappy.interfaces.snapstoreclient import ISnapStoreClient
from lp.soyuz.browser.archive import EnableProcessorsMixin
from lp.soyuz.browser.build import get_build_by_id_str
from lp.soyuz.interfaces.archive import IArchive
@@ -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())
+ 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()
+ 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."""
=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
--- lib/lp/snappy/browser/tests/test_snap.py 2016-04-19 10:15:38 +0000
+++ lib/lp/snappy/browser/tests/test_snap.py 2016-05-11 11:45:30 +0000
@@ -9,17 +9,25 @@
datetime,
timedelta,
)
+import json
import re
from textwrap import dedent
+from urlparse import urlsplit
from fixtures import FakeLogger
+from httmock import (
+ all_requests,
+ HTTMock,
+ )
from mechanize import LinkNotFoundError
+from pymacaroons import Macaroon
import pytz
import soupmatchers
from testtools.matchers import (
MatchesSetwise,
MatchesStructure,
)
+import transaction
from zope.component import getUtility
from zope.publisher.interfaces import NotFound
from zope.security.interfaces import Unauthorized
@@ -31,12 +39,15 @@
from lp.registry.enums import PersonVisibility
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.registry.interfaces.series import SeriesStatus
+from lp.services.config import config
from lp.services.database.constants import UTC_NOW
from lp.services.features.testing import FeatureFixture
from lp.services.webapp import canonical_url
from lp.services.webapp.servers import LaunchpadTestRequest
+from lp.services.webapp.url import urlappend
from lp.snappy.browser.snap import (
SnapAdminView,
+ SnapAuthorizeView,
SnapEditView,
SnapView,
)
@@ -48,6 +59,7 @@
SnapPrivateFeatureDisabled,
)
from lp.testing import (
+ admin_logged_in,
BrowserTestCase,
feature_flags,
login,
@@ -559,6 +571,154 @@
self.assertSnapProcessors(snap, ["386", "armhf"])
+class TestSnapAuthorizeView(BrowserTestCase):
+
+ layer = LaunchpadFunctionalLayer
+
+ def setUp(self):
+ super(TestSnapAuthorizeView, self).setUp()
+ self.useFixture(FeatureFixture(SNAP_TESTING_FLAGS))
+ self.person = self.factory.makePerson(
+ name="test-person", displayname="Test Person")
+ self.distroseries = self.factory.makeUbuntuDistroSeries()
+ with admin_logged_in():
+ self.snappyseries = self.factory.makeSnappySeries(
+ usable_distro_series=[self.distroseries])
+ self.snap = self.factory.makeSnap(
+ registrant=self.person, owner=self.person,
+ distroseries=self.distroseries, store_upload=True,
+ store_series=self.snappyseries,
+ store_name=self.factory.getUniqueUnicode())
+
+ def assertRequestsAuthorization(self, snap, func, *args, **kwargs):
+ root_macaroon = Macaroon()
+ root_macaroon.add_third_party_caveat(
+ urlsplit(config.launchpad.openid_provider_root).netloc, '',
+ 'dummy')
+ root_macaroon_raw = root_macaroon.serialize()
+
+ @all_requests
+ def handler(url, request):
+ self.request = request
+ return {
+ "status_code": 200,
+ "content": {"macaroon": root_macaroon_raw},
+ }
+
+ self.pushConfig("snappy", store_url="http://sca.example/";)
+ with HTTMock(handler):
+ ret = func(*args, **kwargs)
+ self.assertThat(self.request, MatchesStructure.byEquality(
+ url=urlappend(
+ config.snappy.store_url, "api/2.0/acl/package_upload/"),
+ method="POST"))
+ self.assertEqual(
+ {"name": snap.store_name, "series": snap.store_series.name},
+ json.loads(self.request.body))
+ transaction.abort()
+ self.assertEqual({"root": root_macaroon_raw}, snap.store_secrets)
+ return ret
+
+ def test_requestAuthorization(self):
+ with person_logged_in(self.snap.owner):
+ login_url = self.assertRequestsAuthorization(
+ self.snap, SnapAuthorizeView.requestAuthorization,
+ self.snap, LaunchpadTestRequest())
+ self.assertEqual(
+ canonical_url(self.snap) +
+ "/+authorize/+login?field.callback=on&"
+ "macaroon_caveat_id=dummy&"
+ "discharge_macaroon_field=field.discharge_macaroon",
+ login_url)
+
+ def test_unauthorized(self):
+ # A user without edit access cannot authorize snap package uploads.
+ self.useFixture(FakeLogger())
+ other_person = self.factory.makePerson()
+ self.assertRaises(
+ Unauthorized, self.getUserBrowser,
+ canonical_url(self.snap) + "/+authorize", user=other_person)
+
+ def test_no_callback_parameter(self):
+ # If the field.callback parameter is missing, we begin
+ # authorization. This allows (re-)authorizing uploads of an
+ # existing snap package without having to edit it.
+ with person_logged_in(self.snap.owner):
+ view = create_initialized_view(self.snap, "+authorize", form={})
+ self.assertRequestsAuthorization(self.snap, view)
+ self.assertEqual(302, view.request.response.getStatus())
+ self.assertEqual(
+ canonical_url(self.snap) +
+ "/+authorize/+login?field.callback=on&"
+ "macaroon_caveat_id=dummy&"
+ "discharge_macaroon_field=field.discharge_macaroon",
+ view.request.response.getHeader("Location"))
+ self.assertEqual([], view.request.response.notifications)
+
+ def test_missing_root_macaroon(self):
+ # If the snap package has no root macaroon set, we begin
+ # authorization. This allows (re-)authorizing uploads of an
+ # existing snap package without having to edit it.
+ with person_logged_in(self.snap.owner):
+ view = create_initialized_view(
+ self.snap, "+authorize", form={"field.callback": "on"})
+ self.assertRequestsAuthorization(self.snap, view)
+ self.assertEqual(302, view.request.response.getStatus())
+ self.assertEqual(
+ canonical_url(self.snap) +
+ "/+authorize/+login?field.callback=on&"
+ "macaroon_caveat_id=dummy&"
+ "discharge_macaroon_field=field.discharge_macaroon",
+ view.request.response.getHeader("Location"))
+ self.assertEqual([], view.request.response.notifications)
+
+ def test_missing_discharge_macaroon(self):
+ # If field.callback is set but the discharge macaroon is missing, we
+ # indicate an authorization failure.
+ with person_logged_in(self.snap.owner):
+ self.snap.store_secrets = {"root": "root"}
+ view = create_initialized_view(
+ self.snap, "+authorize", form={"field.callback": "on"})
+ self.assertIsNone(view())
+ self.assertEqual(302, view.request.response.getStatus())
+ self.assertEqual(
+ canonical_url(self.snap),
+ view.request.response.getHeader("Location"))
+ self.assertEqual(
+ "Uploads of %s to the store were not authorized." %
+ self.snap.name,
+ view.request.response.notifications[0].message)
+
+ def assertAuthorizationWorks(self, snap, method):
+ with person_logged_in(snap.owner):
+ snap.store_secrets = {"root": "root"}
+ form = {
+ "field.callback": "on",
+ "field.discharge_macaroon": "discharge",
+ }
+ view = create_initialized_view(
+ snap, "+authorize", form=form, method=method)
+ self.assertIsNone(view())
+ self.assertEqual(302, view.request.response.getStatus())
+ self.assertEqual(
+ canonical_url(snap),
+ view.request.response.getHeader("Location"))
+ self.assertEqual(
+ "Uploads of %s to the store are now authorized." % snap.name,
+ view.request.response.notifications[0].message)
+ transaction.abort()
+ self.assertEqual(
+ {"root": "root", "discharge": "discharge"}, snap.store_secrets)
+
+ def test_authorize(self):
+ # Normal authorization works.
+ self.assertAuthorizationWorks(self.snap, "POST")
+
+ def test_authorize_GET(self):
+ # Authorization via a GET request (due to OpenID redirection) works.
+ self.assertAuthorizationWorks(self.snap, "GET")
+
+
class TestSnapDeleteView(BrowserTestCase):
layer = LaunchpadFunctionalLayer
=== modified file 'setup.py'
--- setup.py 2016-05-11 11:45:30 +0000
+++ setup.py 2016-05-11 11:45:30 +0000
@@ -78,6 +78,7 @@
'pgbouncer',
'psycopg2',
'pyasn1',
+ 'pymacaroons',
'pystache',
'python-memcached',
'python-openid',
=== modified file 'versions.cfg'
--- versions.cfg 2016-05-11 11:45:30 +0000
+++ versions.cfg 2016-05-11 11:45:30 +0000
@@ -67,6 +67,7 @@
lazr.sshserver = 0.1.3
lazr.testing = 0.1.1
lazr.uri = 1.0.3
+libnacl = 1.3.6
lpjsmin = 0.5
manuel = 1.7.2
Markdown = 2.3.1
@@ -94,6 +95,7 @@
Pygments = 1.6
pygpgme = 0.2
pyinotify = 0.9.4
+pymacaroons = 0.9.2
pymongo = 2.1.1
pyOpenSSL = 0.13
pystache = 0.5.3
Follow ups
