launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #25245
[Merge] ~cjwatson/launchpad:object-lookup-redirection-2 into launchpad:master
Colin Watson has proposed merging ~cjwatson/launchpad:object-lookup-redirection-2 into launchpad:master.
Commit message:
Make series redirections compatible with object lookup
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/390165
This is another attempt at landing https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/387051, after the previous attempt failed on HTTPS virtual hosts and also broke redirections to the librarian (https://bugs.launchpad.net/launchpad/+bug/1888716).
I fixed the HTTPS virtual host case in lazr.restful (https://code.launchpad.net/~cjwatson/lazr.restful/url-dereferencing-server-url/+merge/390089), and fixed the redirect-to-librarian case by checking whether the redirect target is on one of the main webapp's virtual hosts before trying to look up its context.
Thanks to Thiago F. Pappacena for some of the additional tests here, borrowed from https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/387994.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:object-lookup-redirection-2 into launchpad:master.
diff --git a/constraints.txt b/constraints.txt
index 79ed49e..0bebad0 100644
--- a/constraints.txt
+++ b/constraints.txt
@@ -228,7 +228,7 @@ lazr.delegates==2.0.4
lazr.enum==1.2
lazr.jobrunner==0.16
lazr.lifecycle==1.2
-lazr.restful==0.22.1
+lazr.restful==0.22.2
lazr.restfulclient==0.14.3
lazr.sshserver==0.1.10
lazr.uri==1.0.5
diff --git a/lib/lp/registry/browser/distribution.py b/lib/lp/registry/browser/distribution.py
index 41aa5cd..8d566e0 100644
--- a/lib/lp/registry/browser/distribution.py
+++ b/lib/lp/registry/browser/distribution.py
@@ -190,12 +190,14 @@ class DistributionNavigation(
@stepthrough('+series')
def traverse_series(self, name):
series, _ = self._resolveSeries(name)
- return self.redirectSubTree(canonical_url(series), status=303)
+ return self.redirectSubTree(
+ canonical_url(series, request=self.request), status=303)
def traverse(self, name):
series, is_alias = self._resolveSeries(name)
if is_alias:
- return self.redirectSubTree(canonical_url(series), status=303)
+ return self.redirectSubTree(
+ canonical_url(series, request=self.request), status=303)
else:
return series
diff --git a/lib/lp/registry/browser/product.py b/lib/lp/registry/browser/product.py
index 790ff91..e7dbe7e 100644
--- a/lib/lp/registry/browser/product.py
+++ b/lib/lp/registry/browser/product.py
@@ -287,7 +287,8 @@ class ProductNavigation(
@stepthrough('+series')
def traverse_series(self, name):
series = self.context.getSeries(name)
- return self.redirectSubTree(canonical_url(series), status=303)
+ return self.redirectSubTree(
+ canonical_url(series, request=self.request), status=303)
def traverse(self, name):
return self.context.getSeries(name)
diff --git a/lib/lp/registry/browser/tests/test_distribution.py b/lib/lp/registry/browser/tests/test_distribution.py
index 904462f..d47eb99 100644
--- a/lib/lp/registry/browser/tests/test_distribution.py
+++ b/lib/lp/registry/browser/tests/test_distribution.py
@@ -9,23 +9,31 @@ from __future__ import absolute_import, print_function, unicode_literals
__metaclass__ = type
from fixtures import FakeLogger
-from lazr.restful.interfaces import IJSONRequestCache
+from lazr.restful.fields import Reference
+from lazr.restful.interfaces import (
+ IFieldMarshaller,
+ IJSONRequestCache,
+ )
import soupmatchers
from testtools.matchers import (
MatchesAll,
MatchesAny,
Not,
)
+from zope.component import getMultiAdapter
from zope.schema.vocabulary import SimpleVocabulary
from zope.security.proxy import removeSecurityProxy
from lp.app.browser.lazrjs import vocabulary_to_choice_edit_items
from lp.registry.enums import EXCLUSIVE_TEAM_POLICY
+from lp.registry.interfaces.distroseries import IDistroSeries
from lp.registry.interfaces.ociproject import OCI_PROJECT_ALLOW_CREATE
from lp.registry.interfaces.series import SeriesStatus
from lp.services.features.testing import FeatureFixture
from lp.services.webapp import canonical_url
from lp.services.webapp.publisher import RedirectionView
+from lp.services.webapp.servers import WebServiceTestRequest
+from lp.services.webapp.vhosts import allvhosts
from lp.testing import (
admin_logged_in,
login_celebrity,
@@ -81,6 +89,57 @@ class TestDistributionNavigation(TestCaseWithFactory):
"http://launchpad.test/%s/%s"; % (
distroseries.distribution.name, distroseries.name))
+ def assertDereferences(self, url, expected_obj, environ=None):
+ field = Reference(schema=IDistroSeries)
+ request = WebServiceTestRequest(environ=environ)
+ request.setVirtualHostRoot(names=["devel"])
+ marshaller = getMultiAdapter((field, request), IFieldMarshaller)
+ self.assertIsInstance(marshaller.dereference_url(url), RedirectionView)
+ self.assertEqual(expected_obj, marshaller.marshall_from_json_data(url))
+
+ def test_new_series_url_supports_object_lookup(self):
+ # New-style +series URLs are compatible with webservice object
+ # lookup.
+ distroseries = self.factory.makeDistroSeries()
+ distroseries_url = "/%s/+series/%s" % (
+ distroseries.distribution.name, distroseries.name)
+ self.assertDereferences(distroseries_url, distroseries)
+
+ # Objects subordinate to the redirected series work too.
+ distroarchseries = self.factory.makeDistroArchSeries(
+ distroseries=distroseries)
+ distroarchseries_url = "/%s/+series/%s/%s" % (
+ distroarchseries.distroseries.distribution.name,
+ distroarchseries.distroseries.name,
+ distroarchseries.architecturetag)
+ self.assertDereferences(distroarchseries_url, distroarchseries)
+
+ def test_new_series_url_supports_object_lookup_https(self):
+ # New-style +series URLs are compatible with webservice object
+ # lookup, even if the vhost is configured to use HTTPS.
+ # "SERVER_URL": None exposes a bug in lazr.restful < 0.22.2.
+ self.addCleanup(allvhosts.reload)
+ self.pushConfig("vhosts", use_https=True)
+ allvhosts.reload()
+
+ distroseries = self.factory.makeDistroSeries()
+ distroseries_url = "/%s/+series/%s" % (
+ distroseries.distribution.name, distroseries.name)
+ self.assertDereferences(
+ distroseries_url, distroseries,
+ environ={"HTTPS": "on", "SERVER_URL": None})
+
+ # Objects subordinate to the redirected series work too.
+ distroarchseries = self.factory.makeDistroArchSeries(
+ distroseries=distroseries)
+ distroarchseries_url = "/%s/+series/%s/%s" % (
+ distroarchseries.distroseries.distribution.name,
+ distroarchseries.distroseries.name,
+ distroarchseries.architecturetag)
+ self.assertDereferences(
+ distroarchseries_url, distroarchseries,
+ environ={"HTTPS": "on", "SERVER_URL": None})
+
class TestDistributionPage(TestCaseWithFactory):
"""A TestCase for the distribution index page."""
diff --git a/lib/lp/registry/browser/tests/test_product.py b/lib/lp/registry/browser/tests/test_product.py
index 52544c0..192980c 100644
--- a/lib/lp/registry/browser/tests/test_product.py
+++ b/lib/lp/registry/browser/tests/test_product.py
@@ -9,7 +9,11 @@ __all__ = ['make_product_form']
import re
-from lazr.restful.interfaces import IJSONRequestCache
+from lazr.restful.fields import Reference
+from lazr.restful.interfaces import (
+ IFieldMarshaller,
+ IJSONRequestCache,
+ )
from six.moves.urllib.parse import (
urlencode,
urlsplit,
@@ -24,7 +28,10 @@ from testtools.matchers import (
Not,
)
import transaction
-from zope.component import getUtility
+from zope.component import (
+ getMultiAdapter,
+ getUtility,
+ )
from zope.schema.vocabulary import SimpleVocabulary
from zope.security.proxy import removeSecurityProxy
@@ -49,6 +56,7 @@ from lp.registry.interfaces.product import (
IProductSet,
License,
)
+from lp.registry.interfaces.productseries import IProductSeries
from lp.registry.model.product import Product
from lp.services.config import config
from lp.services.database.interfaces import IStore
@@ -56,6 +64,7 @@ from lp.services.webapp.publisher import (
canonical_url,
RedirectionView,
)
+from lp.services.webapp.servers import WebServiceTestRequest
from lp.services.webapp.vhosts import allvhosts
from lp.testing import (
BrowserTestCase,
@@ -108,6 +117,79 @@ class TestProductNavigation(TestCaseWithFactory):
"http://launchpad.test/%s/%s"; % (
productseries.product.name, productseries.name))
+ def assertNewSeriesURLsDereference(self, environ=None):
+ field = Reference(schema=IProductSeries)
+ request = WebServiceTestRequest()
+ request.setVirtualHostRoot(names=["devel"])
+ marshaller = getMultiAdapter((field, request), IFieldMarshaller)
+ productseries = self.factory.makeProductSeries()
+ productseries_url = "/%s/+series/%s" % (
+ productseries.product.name, productseries.name)
+ resource = marshaller.dereference_url(productseries_url)
+ self.assertIsInstance(resource, RedirectionView)
+ self.assertEqual(
+ productseries,
+ marshaller.marshall_from_json_data(productseries_url))
+
+ # Objects subordinate to the redirected series work too.
+ productrelease = self.factory.makeProductRelease(
+ productseries=productseries)
+ productrelease_url = "/%s/+series/%s/%s" % (
+ productrelease.product.name, productrelease.productseries.name,
+ productrelease.version)
+ self.assertEqual(
+ productrelease,
+ marshaller.marshall_from_json_data(productrelease_url))
+
+ def assertDereferences(self, url, expected_obj, environ=None):
+ field = Reference(schema=IProductSeries)
+ request = WebServiceTestRequest(environ=environ)
+ request.setVirtualHostRoot(names=["devel"])
+ marshaller = getMultiAdapter((field, request), IFieldMarshaller)
+ self.assertIsInstance(marshaller.dereference_url(url), RedirectionView)
+ self.assertEqual(expected_obj, marshaller.marshall_from_json_data(url))
+
+ def test_new_series_url_supports_object_lookup(self):
+ # New-style +series URLs are compatible with webservice object
+ # lookup.
+ productseries = self.factory.makeProductSeries()
+ productseries_url = "/%s/+series/%s" % (
+ productseries.product.name, productseries.name)
+ self.assertDereferences(productseries_url, productseries)
+
+ # Objects subordinate to the redirected series work too.
+ productrelease = self.factory.makeProductRelease(
+ productseries=productseries)
+ productrelease_url = "/%s/+series/%s/%s" % (
+ productrelease.product.name, productrelease.productseries.name,
+ productrelease.version)
+ self.assertDereferences(productrelease_url, productrelease)
+
+ def test_new_series_url_supports_object_lookup_https(self):
+ # New-style +series URLs are compatible with webservice object
+ # lookup, even if the vhost is configured to use HTTPS.
+ # "SERVER_URL": None exposes a bug in lazr.restful < 0.22.2.
+ self.addCleanup(allvhosts.reload)
+ self.pushConfig("vhosts", use_https=True)
+ allvhosts.reload()
+
+ productseries = self.factory.makeProductSeries()
+ productseries_url = "/%s/+series/%s" % (
+ productseries.product.name, productseries.name)
+ self.assertDereferences(
+ productseries_url, productseries,
+ environ={"HTTPS": "on", "SERVER_URL": None})
+
+ # Objects subordinate to the redirected series work too.
+ productrelease = self.factory.makeProductRelease(
+ productseries=productseries)
+ productrelease_url = "/%s/+series/%s/%s" % (
+ productrelease.product.name, productrelease.productseries.name,
+ productrelease.version)
+ self.assertDereferences(
+ productrelease_url, productrelease,
+ environ={"HTTPS": "on", "SERVER_URL": None})
+
class TestProductConfiguration(BrowserTestCase):
"""Tests the configuration links and helpers."""
diff --git a/lib/lp/services/webapp/publisher.py b/lib/lp/services/webapp/publisher.py
index fe0316c..1c19bd8 100644
--- a/lib/lp/services/webapp/publisher.py
+++ b/lib/lp/services/webapp/publisher.py
@@ -1,4 +1,4 @@
-# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Publisher of objects as web pages.
@@ -36,6 +36,7 @@ from lazr.restful import (
)
from lazr.restful.declarations import error_status
from lazr.restful.interfaces import IJSONRequestCache
+from lazr.restful.marshallers import URLDereferencingMixin
from lazr.restful.tales import WebLayerAPI
from lazr.restful.utils import get_current_browser_request
import simplejson
@@ -1081,7 +1082,7 @@ class Navigation:
@implementer(IBrowserPublisher)
-class RedirectionView:
+class RedirectionView(URLDereferencingMixin):
def __init__(self, target, request, status=None, cache_view=None):
self.target = target
@@ -1106,6 +1107,45 @@ class RedirectionView:
def browserDefault(self, request):
return self, ()
+ @property
+ def context(self):
+ """Find the context object corresponding to the redirection target.
+
+ Passing objects as arguments to webservice methods is done by URL,
+ and ObjectLookupFieldMarshaller constructs an internal request to
+ traverse this and find the appropriate object. If the URL in
+ question results in a redirection, then we must recursively traverse
+ the target URL until we find something that the webservice
+ understands.
+ """
+ # Circular import.
+ from lp.services.webapp.servers import WebServiceClientRequest
+
+ if not isinstance(self.request, WebServiceClientRequest):
+ # Raise AttributeError here (as if the "context" attribute
+ # didn't exist) so that e.g.
+ # lp.testing.publication.test_traverse knows to fall back to
+ # other approaches.
+ raise AttributeError(
+ "RedirectionView.context is only supported for webservice "
+ "requests.")
+ parsed_target = urlparse(self.target)
+ if parsed_target.query:
+ raise AttributeError(
+ "RedirectionView.context is not supported for URLs with "
+ "query strings.")
+ # This only supports the canonical root hostname/port for each site,
+ # not any althostnames, but we assume that internally-generated
+ # redirections always use the canonical hostname/port.
+ supported_roots = {
+ urlparse(section.rooturl)[:2]
+ for section in allvhosts.configs.values()}
+ if parsed_target[:2] not in supported_roots:
+ raise AttributeError(
+ "RedirectionView.context is only supported for URLs served "
+ "by the main Launchpad application.")
+ return self.dereference_url_as_object(self.target)
+
@implementer(IBrowserPublisher)
class RenamedView:
diff --git a/lib/lp/services/webapp/tests/test_publisher.py b/lib/lp/services/webapp/tests/test_publisher.py
index 183eab5..5eee7c6 100644
--- a/lib/lp/services/webapp/tests/test_publisher.py
+++ b/lib/lp/services/webapp/tests/test_publisher.py
@@ -1,10 +1,11 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2020 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
from doctest import (
DocTestSuite,
ELLIPSIS,
)
+import io
from unittest import (
TestLoader,
TestSuite,
@@ -24,11 +25,15 @@ from lp.services.webapp.publisher import (
LaunchpadView,
RedirectionView,
)
-from lp.services.webapp.servers import LaunchpadTestRequest
+from lp.services.webapp.servers import (
+ LaunchpadTestRequest,
+ WebServiceClientRequest,
+ )
from lp.services.worlddata.interfaces.country import ICountrySet
from lp.testing import (
login_as,
person_logged_in,
+ TestCase,
TestCaseWithFactory,
)
from lp.testing.layers import DatabaseFunctionalLayer
@@ -410,6 +415,19 @@ class TestLaunchpadView(TestCaseWithFactory):
self.assertEqual(expected_beta_features, view.beta_features)
+class TestRedirectionView(TestCase):
+ layer = DatabaseFunctionalLayer
+
+ def test_redirect_to_non_launchpad_objects(self):
+ request = WebServiceClientRequest(io.BytesIO(b""), {})
+ view = RedirectionView("http://canonical.com";, request)
+ expected_msg = (
+ "RedirectionView.context is only supported for URLs served by the "
+ "main Launchpad application.")
+ self.assertRaisesWithContent(
+ AttributeError, expected_msg, getattr, view, "context")
+
+
def test_suite():
suite = TestSuite()
suite.addTest(DocTestSuite(publisher, optionflags=ELLIPSIS))
diff --git a/lib/lp/soyuz/tests/test_livefsbuild.py b/lib/lp/soyuz/tests/test_livefsbuild.py
index ccd3507..da4de7b 100644
--- a/lib/lp/soyuz/tests/test_livefsbuild.py
+++ b/lib/lp/soyuz/tests/test_livefsbuild.py
@@ -1,4 +1,4 @@
-# Copyright 2014-2019 Canonical Ltd. This software is licensed under the
+# Copyright 2014-2020 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Test live filesystem build features."""
@@ -14,13 +14,14 @@ from datetime import (
from fixtures import FakeLogger
import pytz
+from six.moves.urllib.parse import urlsplit
+from six.moves.urllib.request import urlopen
from testtools.matchers import (
ContainsDict,
Equals,
MatchesDict,
MatchesStructure,
)
-from six.moves.urllib.request import urlopen
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
@@ -554,3 +555,28 @@ class TestLiveFSBuildWebservice(TestCaseWithFactory):
browser.raiseHttpErrors = False
for file_url in file_urls:
self.assertCanOpenRedirectedUrl(browser, file_url)
+
+ def test_read_file_urls_from_webservice(self):
+ # API clients can fetch files attached to builds.
+ db_build = self.factory.makeLiveFSBuild(requester=self.person)
+ db_files = [
+ self.factory.makeLiveFSFile(livefsbuild=db_build)
+ for i in range(2)]
+ build_url = api_url(db_build)
+ file_urls = [
+ ProxiedLibraryFileAlias(file.libraryfile, db_build).http_url
+ for file in db_files]
+ logout()
+ response = self.webservice.named_get(build_url, "getFileUrls")
+ self.assertEqual(200, response.status)
+ self.assertContentEqual(file_urls, response.jsonBody())
+ browser = self.getNonRedirectingBrowser(user=self.person)
+ browser.raiseHttpErrors = False
+
+ for file_url in file_urls:
+ # Make sure we can read the files from the API, following the
+ # redirects.
+ _, _, path, _, _ = urlsplit(file_url)
+ resp = self.webservice.get(path)
+ self.assertEqual(303, resp.status)
+ urlopen(resp.getheader('Location')).close()
