launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #21362
[Merge] lp:~cjwatson/launchpad/generalise-macaroon-auth into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/generalise-macaroon-auth into lp:launchpad.
Commit message:
Generalise snap macaroon handling to allow setting only a root macaroon if the store has provided one without an SSO caveat.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/generalise-macaroon-auth/+merge/315773
This is part of a larger plan to minimise the number of SSO interactions in build.snapcraft.io. The rough plan is to support package_upload_request macaroons in SCA which could be discharged and then used to acquire package_upload macaroons that don't require their own discharge, although this doesn't yet exist in SCA.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/generalise-macaroon-auth into lp:launchpad.
=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py 2017-01-16 22:27:56 +0000
+++ lib/lp/snappy/browser/snap.py 2017-01-27 13:19:31 +0000
@@ -817,7 +817,8 @@
_(u'Uploads of %(snap)s to the store were not authorized.'),
snap=self.context.name))
return
- self.context.completeAuthorization(data['discharge_macaroon'])
+ self.context.completeAuthorization(
+ discharge_macaroon=data['discharge_macaroon'])
self.request.response.addInfoNotification(structured(
_(u'Uploads of %(snap)s to the store are now authorized.'),
snap=self.context.name))
=== modified file 'lib/lp/snappy/browser/tests/test_snapbuild.py'
--- lib/lp/snappy/browser/tests/test_snapbuild.py 2016-07-27 01:43:45 +0000
+++ lib/lp/snappy/browser/tests/test_snapbuild.py 2017-01-27 13:19:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Test snap package build views."""
@@ -9,6 +9,7 @@
from fixtures import FakeLogger
from mechanize import LinkNotFoundError
+from pymacaroons import Macaroon
import soupmatchers
from storm.locals import Store
from testtools.matchers import StartsWith
@@ -246,8 +247,7 @@
with person_logged_in(self.requester):
self.build.snap.store_series = snappyseries
self.build.snap.store_name = self.factory.getUniqueUnicode()
- self.build.snap.store_secrets = {
- "root": "dummy-root", "discharge": "dummy-discharge"}
+ self.build.snap.store_secrets = {"root": Macaroon().serialize()}
def test_store_upload(self):
# A build not previously uploaded to the store can be uploaded
=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py 2017-01-16 22:27:56 +0000
+++ lib/lp/snappy/interfaces/snap.py 2017-01-27 13:19:31 +0000
@@ -372,18 +372,29 @@
"""
@operation_parameters(
+ root_macaroon=TextLine(
+ title=_("Serialized root macaroon"),
+ description=_(
+ "Only required if not already set by beginAuthorization."),
+ required=False),
discharge_macaroon=TextLine(
- title=_("Serialized discharge macaroon"), required=True))
+ title=_("Serialized discharge macaroon"),
+ description=_(
+ "Only required if root macaroon has SSO third-party caveat."),
+ required=False))
@export_write_operation()
@operation_for_version("devel")
- def completeAuthorization(discharge_macaroon):
+ def completeAuthorization(root_macaroon=None, discharge_macaroon=None):
"""Complete authorizing uploads of this snap package to the store.
This is intended for use by third-party sites integrating with
Launchpad.
+ :param root_macaroon: A serialized root macaroon returned by the
+ store. Only required if not already set by beginAuthorization.
:param discharge_macaroon: The serialized discharge macaroon
- returned by SSO via OpenID.
+ returned by SSO via OpenID. Only required if the root macaroon
+ has a third-party caveat addressed to SSO.
:raises CannotAuthorizeStoreUploads: if the snap package is not
properly configured for store uploads.
"""
=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py 2017-01-04 22:59:04 +0000
+++ lib/lp/snappy/model/snap.py 2017-01-27 13:19:31 +0000
@@ -355,13 +355,29 @@
self._store_channels = value or None
@staticmethod
- def extractSSOCaveat(macaroon):
+ def extractSSOCaveats(macaroon):
locations = [
urlsplit(root).netloc
for root in CurrentOpenIDEndPoint.getAllRootURLs()]
- sso_caveats = [
+ return [
c for c in macaroon.third_party_caveats()
if c.location in locations]
+
+ def beginAuthorization(self):
+ """See `ISnap`."""
+ if self.store_series is None:
+ raise CannotAuthorizeStoreUploads(
+ "Cannot authorize uploads of a snap package with no store "
+ "series.")
+ if self.store_name is None:
+ raise CannotAuthorizeStoreUploads(
+ "Cannot authorize uploads of a snap package with no store "
+ "name.")
+ snap_store_client = getUtility(ISnapStoreClient)
+ root_macaroon_raw = snap_store_client.requestPackageUploadPermission(
+ self.store_series, self.store_name)
+ sso_caveats = self.extractSSOCaveats(
+ Macaroon.deserialize(root_macaroon_raw))
# 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
@@ -372,43 +388,38 @@
elif len(sso_caveats) > 1:
raise SnapAuthorizationBadMacaroon(
"Macaroon has multiple SSO caveats")
- return sso_caveats[0]
-
- def beginAuthorization(self):
- """See `ISnap`."""
- if self.store_series is None:
- raise CannotAuthorizeStoreUploads(
- "Cannot authorize uploads of a snap package with no store "
- "series.")
- if self.store_name is None:
- raise CannotAuthorizeStoreUploads(
- "Cannot authorize uploads of a snap package with no store "
- "name.")
- snap_store_client = getUtility(ISnapStoreClient)
- root_macaroon_raw = snap_store_client.requestPackageUploadPermission(
- self.store_series, self.store_name)
- sso_caveat = self.extractSSOCaveat(
- Macaroon.deserialize(root_macaroon_raw))
self.store_secrets = {'root': root_macaroon_raw}
- return sso_caveat.caveat_id
+ return sso_caveats[0].caveat_id
- def completeAuthorization(self, discharge_macaroon):
+ def completeAuthorization(self, root_macaroon=None,
+ discharge_macaroon=None):
"""See `ISnap`."""
- if self.store_secrets is None or "root" not in self.store_secrets:
- raise CannotAuthorizeStoreUploads(
- "beginAuthorization must be called before "
- "completeAuthorization.")
- self.store_secrets["discharge"] = discharge_macaroon
+ if root_macaroon is not None:
+ self.store_secrets = {"root": root_macaroon}
+ else:
+ if self.store_secrets is None or "root" not in self.store_secrets:
+ raise CannotAuthorizeStoreUploads(
+ "beginAuthorization must be called before "
+ "completeAuthorization.")
+ if discharge_macaroon is not None:
+ self.store_secrets["discharge"] = discharge_macaroon
+ else:
+ self.store_secrets.pop("discharge", None)
@property
def can_upload_to_store(self):
- return (
- config.snappy.store_upload_url is not None and
- config.snappy.store_url is not None and
- self.store_series is not None and
- self.store_name is not None and
- self.store_secrets is not None and
- "discharge" in self.store_secrets)
+ if (config.snappy.store_upload_url is None or
+ config.snappy.store_url is None or
+ self.store_series is None or
+ self.store_name is None or
+ self.store_secrets is None or
+ "root" not in self.store_secrets):
+ return False
+ root_macaroon = Macaroon.deserialize(self.store_secrets["root"])
+ if (self.extractSSOCaveats(root_macaroon) and
+ "discharge" not in self.store_secrets):
+ return False
+ return True
def requestBuild(self, requester, archive, distro_arch_series, pocket):
"""See `ISnap`."""
=== modified file 'lib/lp/snappy/model/snapstoreclient.py'
--- lib/lp/snappy/model/snapstoreclient.py 2016-09-19 13:30:27 +0000
+++ lib/lp/snappy/model/snapstoreclient.py 2017-01-27 13:19:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2016 Canonical Ltd. This software is licensed under the
+# Copyright 2016-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Communication with the snap store."""
@@ -81,7 +81,7 @@
# The union of the base64 and URL-safe base64 alphabets.
allowed_chars = set(string.digits + string.letters + "+/=-_")
- def __init__(self, root_macaroon_raw, unbound_discharge_macaroon_raw):
+ def __init__(self, root_macaroon_raw, unbound_discharge_macaroon_raw=None):
self.root_macaroon_raw = root_macaroon_raw
self.unbound_discharge_macaroon_raw = unbound_discharge_macaroon_raw
@@ -108,8 +108,9 @@
def __call__(self, r):
params = []
params.append(self._makeAuthParam("root", self.root_macaroon_raw))
- params.append(
- self._makeAuthParam("discharge", self.discharge_macaroon_raw))
+ if self.unbound_discharge_macaroon_raw is not None:
+ params.append(
+ self._makeAuthParam("discharge", self.discharge_macaroon_raw))
r.headers["Authorization"] = "Macaroon " + ", ".join(params)
return r
@@ -206,7 +207,7 @@
upload_url, method="POST", json=data,
auth=MacaroonAuth(
snap.store_secrets["root"],
- snap.store_secrets["discharge"]))
+ snap.store_secrets.get("discharge")))
response_data = response.json()
return response_data["status_details_url"]
except requests.HTTPError as e:
=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py 2017-01-04 20:52:12 +0000
+++ lib/lp/snappy/tests/test_snap.py 2017-01-27 13:19:31 +0000
@@ -1546,6 +1546,44 @@
discharge_macaroon="dummy-discharge")
self.assertEqual(401, response.status)
+ def test_completeAuthorization_both_macaroons(self):
+ # It is possible to do the authorization work entirely externally
+ # and send both root and discharge macaroons in one go.
+ with admin_logged_in():
+ snappy_series = self.factory.makeSnappySeries()
+ snap = self.factory.makeSnap(
+ registrant=self.person, store_upload=True,
+ store_series=snappy_series,
+ store_name=self.factory.getUniqueUnicode())
+ snap_url = api_url(snap)
+ logout()
+ response = self.webservice.named_post(
+ snap_url, "completeAuthorization",
+ root_macaroon="dummy-root", discharge_macaroon="dummy-discharge")
+ self.assertEqual(200, response.status)
+ with person_logged_in(self.person):
+ self.assertEqual(
+ {"root": "dummy-root", "discharge": "dummy-discharge"},
+ snap.store_secrets)
+
+ def test_completeAuthorization_only_root_macaroon(self):
+ # It is possible to store only a root macaroon. This may make sense
+ # if the store has some other way to determine that user consent has
+ # been acquired and thus has not added a third-party caveat.
+ with admin_logged_in():
+ snappy_series = self.factory.makeSnappySeries()
+ snap = self.factory.makeSnap(
+ registrant=self.person, store_upload=True,
+ store_series=snappy_series,
+ store_name=self.factory.getUniqueUnicode())
+ snap_url = api_url(snap)
+ logout()
+ response = self.webservice.named_post(
+ snap_url, "completeAuthorization", root_macaroon="dummy-root")
+ self.assertEqual(200, response.status)
+ with person_logged_in(self.person):
+ self.assertEqual({"root": "dummy-root"}, snap.store_secrets)
+
def makeBuildableDistroArchSeries(self, **kwargs):
das = self.factory.makeDistroArchSeries(**kwargs)
fake_chroot = self.factory.makeLibraryFileAlias(
=== modified file 'lib/lp/snappy/tests/test_snapbuild.py'
--- lib/lp/snappy/tests/test_snapbuild.py 2016-12-12 13:41:17 +0000
+++ lib/lp/snappy/tests/test_snapbuild.py 2017-01-27 13:19:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Test snap package build features."""
@@ -14,6 +14,7 @@
urlopen,
)
+from pymacaroons import Macaroon
import pytz
from testtools.matchers import (
Equals,
@@ -246,8 +247,7 @@
self.build.snap.store_series = self.factory.makeSnappySeries()
self.build.snap.store_name = self.factory.getUniqueUnicode()
self.build.snap.store_upload = True
- self.build.snap.store_secrets = {
- "root": "dummy-root", "discharge": "dummy-discharge"}
+ self.build.snap.store_secrets = {"root": Macaroon().serialize()}
with dbuser(config.builddmaster.dbuser):
self.build.updateStatus(BuildStatus.FAILEDTOBUILD)
self.assertContentEqual([], self.build.store_upload_jobs)
@@ -257,8 +257,7 @@
self.build.snap.store_series = self.factory.makeSnappySeries()
self.build.snap.store_name = self.factory.getUniqueUnicode()
self.build.snap.store_upload = True
- self.build.snap.store_secrets = {
- "root": "dummy-root", "discharge": "dummy-discharge"}
+ self.build.snap.store_secrets = {"root": Macaroon().serialize()}
with dbuser(config.builddmaster.dbuser):
self.build.updateStatus(BuildStatus.FULLYBUILT)
self.assertEqual(1, len(list(self.build.store_upload_jobs)))
@@ -360,8 +359,7 @@
self.build.snap.store_series = self.factory.makeSnappySeries(
usable_distro_series=[self.build.snap.distro_series])
self.build.snap.store_name = self.factory.getUniqueUnicode()
- self.build.snap.store_secrets = {
- "root": "dummy-root", "discharge": "dummy-discharge"}
+ self.build.snap.store_secrets = {"root": Macaroon().serialize()}
def test_scheduleStoreUpload(self):
# A build not previously uploaded to the store can be uploaded
=== modified file 'lib/lp/snappy/tests/test_snapbuildbehaviour.py'
--- lib/lp/snappy/tests/test_snapbuildbehaviour.py 2016-12-02 12:53:41 +0000
+++ lib/lp/snappy/tests/test_snapbuildbehaviour.py 2017-01-27 13:19:31 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2017 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Test snap package build behaviour."""
@@ -14,6 +14,7 @@
Mock,
patch,
)
+from pymacaroons import Macaroon
from testtools import ExpectedException
from testtools.deferredruntest import AsynchronousDeferredRunTest
from testtools.matchers import IsInstance
@@ -328,8 +329,7 @@
distroseries=distroseries, store_upload=True,
store_series=snappyseries,
store_name=self.factory.getUniqueUnicode(),
- store_secrets={
- "root": "dummy-root", "discharge": "dummy-discharge"})
+ store_secrets={"root": Macaroon().serialize()})
def makeBuild(self):
snap = self.makeSnap()
=== modified file 'lib/lp/snappy/tests/test_snapstoreclient.py'
--- lib/lp/snappy/tests/test_snapstoreclient.py 2017-01-04 20:52:12 +0000
+++ lib/lp/snappy/tests/test_snapstoreclient.py 2017-01-27 13:19:31 +0000
@@ -28,7 +28,6 @@
Contains,
ContainsDict,
Equals,
- KeysEqual,
Matcher,
MatchesDict,
MatchesListwise,
@@ -74,13 +73,17 @@
self.key = key
def match(self, macaroons):
- mismatch = KeysEqual("root", "discharge").match(macaroons)
+ mismatch = Contains("root").match(macaroons)
if mismatch is not None:
return mismatch
root_macaroon = Macaroon.deserialize(macaroons["root"])
- discharge_macaroon = Macaroon.deserialize(macaroons["discharge"])
+ if "discharge" in macaroons:
+ discharge_macaroons = [
+ Macaroon.deserialize(macaroons["discharge"])]
+ else:
+ discharge_macaroons = []
try:
- Verifier().verify(root_macaroon, self.key, [discharge_macaroon])
+ Verifier().verify(root_macaroon, self.key, discharge_macaroons)
except Exception as e:
return Mismatch("Macaroons do not verify: %s" % e)
@@ -107,6 +110,17 @@
parse_dict_header(auth_value[len("Macaroon "):]),
MacaroonsVerify(root_key))
+ def test_good_no_discharge(self):
+ r = Request()
+ root_key = hashlib.sha256("root").hexdigest()
+ root_macaroon = Macaroon(key=root_key)
+ MacaroonAuth(root_macaroon.serialize())(r)
+ auth_value = r.headers["Authorization"]
+ self.assertThat(auth_value, StartsWith("Macaroon "))
+ self.assertThat(
+ parse_dict_header(auth_value[len("Macaroon "):]),
+ MacaroonsVerify(root_key))
+
def test_bad_framing(self):
r = Request()
self.assertRaises(
Follow ups
