launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #21298
[Merge] lp:~cjwatson/launchpad/snap-api-complete-authorization into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-api-complete-authorization into lp:launchpad.
Commit message:
Rearrange API-based snap authorization: Snap.beginAuthorization no longer takes a URL and now returns a caveat ID, and clients now need to call Snap.completeAuthorization too.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-api-complete-authorization/+merge/313396
I realised today that the previous design of Snap.beginAuthorization was unworkable for third-party sites such as build.snapcraft.io where the client browser is not expected to possess Launchpad credentials, because the end of the OpenID exchange required being able to use Snap:+authorize, which requires launchpad.Edit on the Snap.
Instead of relying on Launchpad's own OpenID exchange in this case, a much simpler solution (and also one that will probably integrate better with other sites) is to return only the information needed to start the OpenID exchange and rely on the calling site to run the whole thing itself. This requires Snap.beginAuthorization to return the SSO caveat ID to the caller (which it already did, just embedded in a URL), and for there to be a new Snap.completeAuthorization method that the calling site is required to call with the discharge macaroon when the exchange is complete.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-api-complete-authorization into lp:launchpad.
=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py 2016-12-08 17:39:19 +0000
+++ lib/lp/snappy/browser/snap.py 2016-12-15 20:23:14 +0000
@@ -16,6 +16,8 @@
'SnapView',
]
+from urllib import urlencode
+
from lazr.restful.fields import Reference
from lazr.restful.interface import (
copy_field,
@@ -56,7 +58,6 @@
from lp.registry.enums import VCSType
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.services.features import getFeatureFlag
-from lp.services.fields import URIField
from lp.services.helpers import english_list
from lp.services.propertycache import cachedproperty
from lp.services.scripts import log
@@ -76,6 +77,7 @@
Breadcrumb,
NameBreadcrumb,
)
+from lp.services.webapp.url import urlappend
from lp.services.webhooks.browser import WebhookTargetNavigationMixin
from lp.snappy.browser.widgets.snaparchive import SnapArchiveWidget
from lp.snappy.interfaces.snap import (
@@ -777,8 +779,6 @@
discharge_macaroon = TextLine(
title=u'Serialized discharge macaroon', required=True)
- success_url = URIField(
- title=u'URL to redirect to on success', allowed_schemes=['https'])
render_context = False
@@ -792,7 +792,15 @@
def requestAuthorization(cls, snap, request):
"""Begin the process of authorizing uploads of a snap package."""
try:
- return snap.beginAuthorization()
+ sso_caveat_id = snap.beginAuthorization()
+ base_url = canonical_url(snap, view_name='+authorize')
+ login_url = urlappend(base_url, '+login')
+ login_url += '?%s' % urlencode([
+ ('macaroon_caveat_id', sso_caveat_id),
+ ('discharge_macaroon_action', 'field.actions.complete'),
+ ('discharge_macaroon_field', 'field.discharge_macaroon'),
+ ])
+ return login_url
except CannotAuthorizeStoreUploads as e:
request.response.addInfoNotification(unicode(e))
request.response.redirect(canonical_url(snap))
@@ -811,20 +819,11 @@
_(u'Uploads of %(snap)s to the store were not authorized.'),
snap=self.context.name))
return
- success_url = data.get('success_url')
- # XXX cjwatson 2016-12-08: Drop this once all appservers have been
- # upgraded to always pass success_url.
- if success_url is None:
- success_url = canonical_url(self.context)
- # 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
+ self.context.completeAuthorization(data['discharge_macaroon'])
self.request.response.addInfoNotification(structured(
_(u'Uploads of %(snap)s to the store are now authorized.'),
snap=self.context.name))
- self.request.response.redirect(success_url)
+ self.request.response.redirect(canonical_url(self.context))
@property
def adapters(self):
=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
--- lib/lp/snappy/browser/tests/test_snap.py 2016-12-08 17:39:19 +0000
+++ lib/lp/snappy/browser/tests/test_snap.py 2016-12-15 20:23:14 +0000
@@ -11,7 +11,6 @@
)
import json
import re
-from urllib import quote_plus
from urllib2 import HTTPError
from urlparse import (
parse_qs,
@@ -433,7 +432,6 @@
"discharge_macaroon_action": ["field.actions.complete"],
"discharge_macaroon_field": ["field.discharge_macaroon"],
"macaroon_caveat_id": ["dummy"],
- "field.success_url": [canonical_url(snap, rootsite="code")],
}
self.assertEqual(expected_args, parse_qs(parsed_location[3]))
@@ -974,7 +972,6 @@
"discharge_macaroon_action": ["field.actions.complete"],
"discharge_macaroon_field": ["field.discharge_macaroon"],
"macaroon_caveat_id": ["dummy"],
- "field.success_url": [canonical_url(snap)],
}
self.assertEqual(expected_args, parse_qs(parsed_location[3]))
@@ -1049,8 +1046,7 @@
self.assertEqual(
snap_url + "/+authorize/+login?macaroon_caveat_id=dummy&"
"discharge_macaroon_action=field.actions.complete&"
- "discharge_macaroon_field=field.discharge_macaroon&"
- "field.success_url=" + quote_plus(snap_url),
+ "discharge_macaroon_field=field.discharge_macaroon",
redirection.hdrs["Location"])
def test_complete_authorization_missing_discharge_macaroon(self):
@@ -1096,32 +1092,6 @@
{"root": "root", "discharge": "discharge"},
self.snap.store_secrets)
- def test_complete_authorization_with_success_url(self):
- # The "complete" action honours the provided success_url.
- with person_logged_in(self.snap.owner):
- self.snap.store_secrets = {"root": "root"}
- transaction.commit()
- form = {
- "field.actions.complete": "1",
- "field.discharge_macaroon": "discharge",
- "field.success_url": "https://example.org/";,
- }
- view = create_initialized_view(
- self.snap, "+authorize", form=form, method="POST",
- principal=self.snap.owner)
- self.assertEqual("", view())
- self.assertEqual(302, view.request.response.getStatus())
- self.assertEqual(
- "https://example.org/";,
- view.request.response.getHeader("Location"))
- self.assertEqual(
- "Uploads of %s to the store are now authorized." %
- self.snap.name,
- view.request.response.notifications[0].message)
- self.assertEqual(
- {"root": "root", "discharge": "discharge"},
- self.snap.store_secrets)
-
class TestSnapDeleteView(BrowserTestCase):
=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py 2016-12-08 17:50:35 +0000
+++ lib/lp/snappy/interfaces/snap.py 2016-12-15 20:23:14 +0000
@@ -347,20 +347,17 @@
:return: A sequence of `ISnapBuild` instances.
"""
- @operation_parameters(
- success_url=URIField(
- title=_("URL to redirect to on success"),
- allowed_schemes=["https"]))
@export_write_operation()
@operation_for_version("devel")
- def beginAuthorization(success_url=None):
+ def beginAuthorization():
"""Begin authorizing uploads of this snap package to the store.
This is intended for use by third-party sites integrating with
Launchpad. Most users should visit <snap URL>/+authorize instead.
:param success_url: The URL to redirect to when authorization is
- complete. Defaults to the canonical URL of the snap.
+ complete. If None (only allowed for internal use), defaults to
+ the canonical URL of the snap.
:raises CannotAuthorizeStoreUploads: if the snap package is not
properly configured for store uploads.
:raises BadRequestPackageUploadResponse: if the store returns an
@@ -368,8 +365,27 @@
package_upload macaroon.
:raises SnapAuthorizationBadMacaroon: if the package_upload macaroon
returned by the store has unsuitable SSO caveats.
- :return: A URL that the third-party site should redirect the user to
- in order to continue authorization.
+ :return: The SSO caveat ID from the package_upload macaroon returned
+ by the store. The third-party site should acquire a discharge
+ macaroon for this caveat using OpenID and then call
+ `completeAuthorization`.
+ """
+
+ @operation_parameters(
+ discharge_macaroon=TextLine(
+ title=_("Serialized discharge macaroon"), required=True))
+ @export_write_operation()
+ @operation_for_version("devel")
+ def completeAuthorization(discharge_macaroon):
+ """Complete authorizing uploads of this snap package to the store.
+
+ This is intended for use by third-party sites integrating with
+ Launchpad.
+
+ :param discharge_macaroon: The serialized discharge macaroon
+ returned by SSO via OpenID.
+ :raises CannotAuthorizeStoreUploads: if the snap package is not
+ properly configured for store uploads.
"""
@export_destructor_operation()
=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py 2016-12-08 17:39:19 +0000
+++ lib/lp/snappy/model/snap.py 2016-12-15 20:23:14 +0000
@@ -10,7 +10,6 @@
datetime,
timedelta,
)
-from urllib import urlencode
from urlparse import urlsplit
from pymacaroons import Macaroon
@@ -102,8 +101,6 @@
)
from lp.services.openid.adapters.openid import CurrentOpenIDEndPoint
from lp.services.webapp.interfaces import ILaunchBag
-from lp.services.webapp.publisher import canonical_url
-from lp.services.webapp.url import urlappend
from lp.services.webhooks.interfaces import IWebhookSet
from lp.services.webhooks.model import WebhookTargetMixin
from lp.snappy.interfaces.snap import (
@@ -369,7 +366,7 @@
"Macaroon has multiple SSO caveats")
return sso_caveats[0]
- def beginAuthorization(self, success_url=None):
+ def beginAuthorization(self):
"""See `ISnap`."""
if self.store_series is None:
raise CannotAuthorizeStoreUploads(
@@ -379,23 +376,21 @@
raise CannotAuthorizeStoreUploads(
"Cannot authorize uploads of a snap package with no store "
"name.")
- if success_url is None:
- success_url = canonical_url(self)
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}
- base_url = canonical_url(self, view_name='+authorize')
- login_url = urlappend(base_url, '+login')
- login_url += '?%s' % urlencode([
- ('macaroon_caveat_id', sso_caveat.caveat_id),
- ('discharge_macaroon_action', 'field.actions.complete'),
- ('discharge_macaroon_field', 'field.discharge_macaroon'),
- ('field.success_url', success_url),
- ])
- return login_url
+ return sso_caveat.caveat_id
+
+ def completeAuthorization(self, discharge_macaroon):
+ """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
@property
def can_upload_to_store(self):
=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py 2016-12-08 17:39:19 +0000
+++ lib/lp/snappy/tests/test_snap.py 2016-12-15 20:23:14 +0000
@@ -10,7 +10,6 @@
timedelta,
)
import json
-from urllib import quote_plus
from urlparse import urlsplit
from httmock import (
@@ -55,7 +54,6 @@
from lp.services.log.logger import BufferLogger
from lp.services.propertycache import clear_property_cache
from lp.services.webapp.interfaces import OAuthPermission
-from lp.services.webapp.publisher import canonical_url
from lp.snappy.interfaces.snap import (
BadSnapSearchContext,
CannotModifySnapProcessor,
@@ -1440,7 +1438,7 @@
}
self.assertEqual(expected_body, json.loads(self.request.body))
self.assertEqual({"root": root_macaroon_raw}, snap.store_secrets)
- return response
+ return response, root_macaroon.third_party_caveats()[0]
def test_beginAuthorization(self):
with admin_logged_in():
@@ -1449,31 +1447,8 @@
registrant=self.person, store_upload=True,
store_series=snappy_series,
store_name=self.factory.getUniqueUnicode())
- snap_url = canonical_url(snap)
- response = self.assertBeginsAuthorization(snap)
- self.assertEqual(
- snap_url + "/+authorize/+login?macaroon_caveat_id=dummy&"
- "discharge_macaroon_action=field.actions.complete&"
- "discharge_macaroon_field=field.discharge_macaroon&"
- "field.success_url=" + quote_plus(snap_url),
- response.jsonBody())
-
- def test_beginAuthorization_with_success_url(self):
- 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 = canonical_url(snap)
- response = self.assertBeginsAuthorization(
- snap, success_url="https://example.org/";)
- self.assertEqual(
- snap_url + "/+authorize/+login?macaroon_caveat_id=dummy&"
- "discharge_macaroon_action=field.actions.complete&"
- "discharge_macaroon_field=field.discharge_macaroon&"
- "field.success_url=https%3A%2F%2Fexample.org%2F",
- response.jsonBody())
+ response, sso_caveat = self.assertBeginsAuthorization(snap)
+ self.assertEqual(sso_caveat.caveat_id, response.jsonBody())
def test_beginAuthorization_unauthorized(self):
# A user without edit access cannot authorize snap package uploads.
@@ -1491,6 +1466,61 @@
response = other_webservice.named_post(snap_url, "beginAuthorization")
self.assertEqual(401, response.status)
+ def test_completeAuthorization(self):
+ 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(),
+ store_secrets={"root": "dummy-root"})
+ snap_url = api_url(snap)
+ logout()
+ response = self.webservice.named_post(
+ snap_url, "completeAuthorization",
+ discharge_macaroon="dummy-discharge")
+ self.assertEqual(200, response.status)
+ self.assertEqual("", response.body)
+ with person_logged_in(self.person):
+ self.assertEqual(
+ {"root": "dummy-root", "discharge": "dummy-discharge"},
+ snap.store_secrets)
+
+ def test_completeAuthorization_without_beginAuthorization(self):
+ 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",
+ discharge_macaroon="dummy-discharge")
+ self.assertEqual(400, response.status)
+ self.assertEqual(
+ "beginAuthorization must be called before completeAuthorization.",
+ response.body)
+
+ def test_completeAuthorization_unauthorized(self):
+ 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(),
+ store_secrets={"root": "dummy-root"})
+ snap_url = api_url(snap)
+ other_person = self.factory.makePerson()
+ other_webservice = webservice_for_person(
+ other_person, permission=OAuthPermission.WRITE_PUBLIC)
+ other_webservice.default_api_version = "devel"
+ response = other_webservice.named_post(
+ snap_url, "completeAuthorization",
+ discharge_macaroon="dummy-discharge")
+ self.assertEqual(401, response.status)
+
def makeBuildableDistroArchSeries(self, **kwargs):
das = self.factory.makeDistroArchSeries(**kwargs)
fake_chroot = self.factory.makeLibraryFileAlias(
Follow ups
