launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:fix-publisher-context-for-external-redirects into launchpad:master.

Commit message:
Fixing context for RedirectionView when target is not a Launchpad object

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1888716 in Launchpad itself: "OOPS trying to download livefs build logs (buildd images)"
  https://bugs.launchpad.net/launchpad/+bug/1888716

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/387994
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:fix-publisher-context-for-external-redirects into launchpad:master.

diff --git a/lib/lp/code/templates/gitref-pending-merges.pt b/lib/lp/code/templates/gitref-pending-merges.pt
index 5d41ce8..4750b68 100644
--- a/lib/lp/code/templates/gitref-pending-merges.pt
+++ b/lib/lp/code/templates/gitref-pending-merges.pt
@@ -46,7 +46,7 @@
     <div tal:condition="python: view.propose_merge_notes and context_menu['register_merge'].enabled">
       <ul>
         <li tal:repeat="message view/propose_merge_notes" class="registered">
-           <spam tal:replace="message" />
+           <span tal:replace="message" />
         </li>
       </ul>
     </div>
diff --git a/lib/lp/services/webapp/publisher.py b/lib/lp/services/webapp/publisher.py
index f01466d..c12c7e4 100644
--- a/lib/lp/services/webapp/publisher.py
+++ b/lib/lp/services/webapp/publisher.py
@@ -1133,7 +1133,12 @@ class RedirectionView(URLDereferencingMixin):
             raise AttributeError(
                 "RedirectionView.context is not supported for URLs with "
                 "query strings.")
-        return self.dereference_url_as_object(self.target)
+        try:
+            return self.dereference_url_as_object(self.target)
+        except ValueError:
+            raise AttributeError(
+                "RedirectionView.context is only supported for Launchpad "
+                "objects as target. Not %s." % self.target)
 
 
 @implementer(IBrowserPublisher)
diff --git a/lib/lp/services/webapp/tests/test_publisher.py b/lib/lp/services/webapp/tests/test_publisher.py
index 183eab5..5f04e14 100644
--- a/lib/lp/services/webapp/tests/test_publisher.py
+++ b/lib/lp/services/webapp/tests/test_publisher.py
@@ -1,4 +1,4 @@
-# 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 (
@@ -16,6 +16,7 @@ from zope.component import getUtility
 from zope.interface import implementer
 
 from lp.app.interfaces.launchpad import IPrivacy
+from lp.services.compat import mock
 from lp.services.features.flags import flag_info
 from lp.services.features.testing import FeatureFixture
 from lp.services.webapp import publisher
@@ -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,20 @@ 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(mock.Mock(), {})
+        view = RedirectionView("http://canonical.com";, request)
+        expected_msg = (
+            "RedirectionView.context is only supported for Launchpad "
+            "objects as target. Not http://canonical.com.";)
+        self.assertRaisesRegex(
+            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()