From 765c39ef9864aa592a2cbcf642fbdfac264b399b Mon Sep 17 00:00:00 2001 From: Lukas Graf Date: Sun, 24 Jun 2018 18:02:11 +0200 Subject: [PATCH] Add support for redirects from plone.app.redirector: plone.rest will now handle redirects created by ``plone.app.redirector`` pretty much the same way as regular Plone. If a redirect exists for a given URL, a ``GET`` request will be answered with ``301``, and the new location for the resource is indicated in the ``Location`` header. Any other request method than GET (``POST``, ``PATCH``, ...) will be answered with ``308 Permanent Redirect``. This status code instructs the client that it should NOT switch the method, but retry (if desired) the request with the *same* method at the new location. --- CHANGES.rst | 5 +- README.rst | 24 +++ src/plone/rest/errors.py | 147 +++++++++++++++++++ src/plone/rest/patches.py | 22 +++ src/plone/rest/patches.zcml | 8 + src/plone/rest/tests/test_dispatching.py | 155 +++++++++++++++++++- src/plone/rest/tests/test_redirects.py | 177 +++++++++++++++++++++++ 7 files changed, 534 insertions(+), 4 deletions(-) create mode 100644 src/plone/rest/tests/test_redirects.py diff --git a/CHANGES.rst b/CHANGES.rst index cdb76b6..92935e1 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -1,10 +1,11 @@ Changelog ========= -1.1.2 (unreleased) +1.2.0 (unreleased) ------------------ -- Nothing changed yet. +- Add support for redirects from plone.app.redirector. + [lgraf] 1.1.1 (2018-06-22) diff --git a/README.rst b/README.rst index 3af2a40..6a71e45 100644 --- a/README.rst +++ b/README.rst @@ -287,6 +287,30 @@ Install plone.rest by adding it to your buildout:: and then running "bin/buildout" +Redirects +--------- + +plone.rest will handle redirects created by ``plone.app.redirector`` pretty +much the same way as regular Plone. + +If a redirect exists for a given URL, a ``GET`` request will be answered with +``301``, and the new location for the resource is indicated in the ``Location`` +header:: + + HTTP/1.1 301 Moved Permanently + + Content-Type: application/json + Location: http://localhost:8080/Plone/my-folder-new-location + +Any other request method than GET (``POST``, ``PATCH``, ...) will be answered +with ``308 Permanent Redirect``. This status code instructs the client that +it should NOT switch the method, but retry (if desired) the request with the +*same* method at the new location. + +In practice, both the Python ``requests`` library a well as Postman seem to +honour this behavior be default. + + Contribute ---------- diff --git a/src/plone/rest/errors.py b/src/plone/rest/errors.py index fa8e3f5..0bc567d 100644 --- a/src/plone/rest/errors.py +++ b/src/plone/rest/errors.py @@ -1,9 +1,15 @@ from AccessControl import getSecurityManager +from plone.app.redirector.interfaces import IRedirectionStorage +from plone.memoize.instance import memoize from plone.rest.interfaces import IAPIRequest from Products.CMFCore.permissions import ManagePortal from Products.Five.browser import BrowserView +from six.moves import urllib +from six.moves.urllib.parse import quote +from six.moves.urllib.parse import unquote from zExceptions import NotFound from zope.component import adapter +from zope.component import queryUtility from zope.component.hooks import getSite import json @@ -13,6 +19,9 @@ @adapter(Exception, IAPIRequest) class ErrorHandling(BrowserView): + """This view is responsible for serializing unhandled exceptions, as well + as handling 404 Not Found errors and redirects. + """ def __call__(self): exception = self.context @@ -37,6 +46,12 @@ def render_exception(self, exception): u'message': str(exception).decode('utf-8')} if isinstance(exception, NotFound): + # First check if a redirect from p.a.redirector exists + redirect_performed = self.attempt_redirect() + if redirect_performed: + self.request.response.setBody('', lock=1) + return + # NotFound exceptions need special handling because their # exception message gets turned into HTML by ZPublisher url = self.request.getURL() @@ -55,3 +70,135 @@ def render_traceback(self, exception): raw = '\n'.join(traceback.format_tb(exc_traceback)) return raw.strip().split('\n') + + def find_redirect_if_view_or_service(self, old_path_elements, storage): + """Find redirect for URLs like: + - http://example.com/object/namedservice/param + - http://example.com/object/@@view/param + - http://example.com/object/template + + This combines the functionality of the find_redirect_if_view() and + find_redirect_if_template() methods of the original FourOhFourView into + one, and also makes it support named services. + + For this to also work for named services we use a different strategy + here: Based on old_path_elements, try to find the longest stored + redirect (if any), and consider the remaining path parts the remainder + (view, template, named services plus possible params) that will need + to be appended to the new object path. + """ + if len(old_path_elements) <= 1: + return None + + # Parts to the left of the split point are considered a potential + # object path, while the right part is the remainder. Starting from + # the right (longest potential obj path), we keep moving the split + # point to the left and look for shorter matches. + # + # Once we reach the point where the obj path is separated from the + # remainder, we should get a match if there's a stored redirect. + # + # ['', 'Plone', 'folder', 'item', '@@view', 'param'] + # ^ + splitpoint = len(old_path_elements) + + while splitpoint > 1: + possible_obj_path = '/'.join(old_path_elements[:splitpoint]) + remainder = old_path_elements[splitpoint:] + new_path = storage.get(possible_obj_path) + + if new_path: + if new_path == possible_obj_path: + # New URL would match originally requested URL. + # Lets not cause a redirect loop. + return None + return new_path + '/' + '/'.join(remainder) + + splitpoint -= 1 + + return None + + def attempt_redirect(self): + """Check if a redirect is needed, and perform it if necessary. + + Returns True if a redirect has been performed, False otherwise. + + This method is based on FourOhFourView.attempt_redirect() from + p.a.redirector. It's copied here because we want to answer redirects + to non-GET methods with status 308, but since this method locks the + response status, we wouldn't be able to change it afterwards. + """ + url = self._url() + if not url: + return False + + try: + old_path_elements = self.request.physicalPathFromURL(url) + except ValueError: + return False + + storage = queryUtility(IRedirectionStorage) + if storage is None: + return False + + old_path = '/'.join(old_path_elements) + + # First lets try with query string in cases or content migration + + new_path = None + + query_string = self.request.QUERY_STRING + if query_string: + new_path = storage.get("%s?%s" % (old_path, query_string)) + # if we matched on the query_string we don't want to include it + # in redirect + if new_path: + query_string = '' + + if not new_path: + new_path = storage.get(old_path) + + # Attempt our own strategy at finding redirects for named REST + # services, views or templates. + if not new_path: + new_path = self.find_redirect_if_view_or_service( + old_path_elements, storage) + + if not new_path: + return False + + url = urllib.parse.urlsplit(new_path) + if url.netloc: + # External URL + # avoid double quoting + url_path = unquote(url.path) + url_path = quote(url_path) + url = urllib.parse.SplitResult( + *(url[:2] + (url_path, ) + url[3:])).geturl() + else: + url = self.request.physicalPathToURL(new_path) + + # some analytics programs might use this info to track + if query_string: + url += "?" + query_string + + # Answer GET requests with 301. Every other method will be answered + # with 308 Permanent Redirect, which instructs the client to NOT + # switch the method (if the original request was a POST, it should + # re-POST to the new URL from the Location header). + if self.request.method.upper() == 'GET': + status = 301 + else: + status = 308 + + self.request.response.redirect(url, status=status, lock=1) + return True + + @memoize + def _url(self): + """Get the current, canonical URL + """ + return self.request.get('ACTUAL_URL', + self.request.get('VIRTUAL_URL', # noqa + self.request.get('URL', # noqa + None))) # noqa diff --git a/src/plone/rest/patches.py b/src/plone/rest/patches.py index c5aedeb..bf24c1b 100644 --- a/src/plone/rest/patches.py +++ b/src/plone/rest/patches.py @@ -13,3 +13,25 @@ def __before_publishing_traverse__(self, arg1, arg2=None): return return self._old___before_publishing_traverse__(arg1, arg2) + + +PERMANENT_REDIRECT = {308: 'Permanent Redirect'} + + +def patch_zpublisher_status_codes(scope, unused_original, unused_replacement): + """Add '308 Permanent Redirect' to the list of status codes the ZPublisher + knows about. Otherwise setStatus() will turn it into a 500. + """ + # Patch the forward mapping (code -> reason) + status_reasons = getattr(scope, 'status_reasons') + if 308 not in status_reasons: + status_reasons.update(PERMANENT_REDIRECT) + + # Update the reverse mapping + status_codes = getattr(scope, 'status_codes') + key, val = PERMANENT_REDIRECT.items()[0] + + status_codes[''.join(val.split(' ')).lower()] = key + status_codes[val.lower()] = key + status_codes[key] = key + status_codes[str(key)] = key diff --git a/src/plone/rest/patches.zcml b/src/plone/rest/patches.zcml index 54aa24c..ff6f7a6 100644 --- a/src/plone/rest/patches.zcml +++ b/src/plone/rest/patches.zcml @@ -13,4 +13,12 @@ preserveOriginal="true" /> + + diff --git a/src/plone/rest/tests/test_dispatching.py b/src/plone/rest/tests/test_dispatching.py index ca83c65..171ed72 100644 --- a/src/plone/rest/tests/test_dispatching.py +++ b/src/plone/rest/tests/test_dispatching.py @@ -27,7 +27,7 @@ def setUp(self): self.portal_url = self.portal.absolute_url() setRoles(self.portal, TEST_USER_ID, ['Manager']) - def validate(self, expectations): + def validate(self, expectations, follow_redirects=False): failures = [] for expectation in expectations: path, method, creds, expected_status = expectation @@ -37,10 +37,12 @@ def validate(self, expectations): method, url, headers={'Accept': 'application/json'}, auth=creds, + allow_redirects=follow_redirects, ) if response.status_code != expected_status: - request_args = (path, method, creds) + request_args = (path, method, creds, + '(follow_redirects=%s)' % follow_redirects) actual_status = response.status_code failure = (request_args, expected_status, actual_status) failures.append(failure) @@ -207,3 +209,152 @@ def test_public_dx_folder_invalid_creds(self): ('/public', 'OPTIONS', INVALID_CREDS, 200), ] self.validate(expectations) + + +class TestDispatchingRedirects(DispatchingTestCase): + + def setUp(self): + super(TestDispatchingRedirects, self).setUp() + + self.portal.invokeFactory('Folder', id='private-old') + self.portal.manage_renameObject('private-old', 'private-new') + + self.portal.invokeFactory('Folder', id='public-old') + public_folder = self.portal['public-old'] + wftool = getToolByName(self.portal, "portal_workflow") + wftool.doActionFor(public_folder, "publish") + self.portal.manage_renameObject('public-old', 'public-new') + + transaction.commit() + + def test_moved_private_dx_folder_with_creds(self): + expectations = [ + ('/private-old', 'GET', CREDS, 301), + ('/private-old', 'POST', CREDS, 308), + ('/private-old', 'PUT', CREDS, 308), + ('/private-old', 'PATCH', CREDS, 308), + ('/private-old', 'DELETE', CREDS, 308), + ('/private-old', 'OPTIONS', CREDS, 308), + ] + self.validate(expectations) + + # Same, but with following redirects, asserting on the final status + expectations = [ + ('/private-old', 'GET', CREDS, 200), + ('/private-old', 'POST', CREDS, 200), + ('/private-old', 'PUT', CREDS, 200), + ('/private-old', 'PATCH', CREDS, 200), + ('/private-old', 'DELETE', CREDS, 200), + ('/private-old', 'OPTIONS', CREDS, 200), + ] + self.validate(expectations, follow_redirects=True) + + def test_moved_private_dx_folder_without_creds(self): + expectations = [ + ('/private-old', 'GET', NO_CREDS, 301), + ('/private-old', 'POST', NO_CREDS, 308), + ('/private-old', 'PUT', NO_CREDS, 308), + ('/private-old', 'PATCH', NO_CREDS, 308), + ('/private-old', 'DELETE', NO_CREDS, 308), + ('/private-old', 'OPTIONS', NO_CREDS, 308), + ] + self.validate(expectations) + + # Same, but with following redirects, asserting on the final status + expectations = [ + ('/private-old', 'GET', NO_CREDS, 401), + ('/private-old', 'POST', NO_CREDS, 401), + ('/private-old', 'PUT', NO_CREDS, 401), + ('/private-old', 'PATCH', NO_CREDS, 401), + ('/private-old', 'DELETE', NO_CREDS, 401), + ('/private-old', 'OPTIONS', NO_CREDS, 401), + ] + self.validate(expectations, follow_redirects=True) + + def test_moved_private_dx_folder_invalid_creds(self): + expectations = [ + ('/private-old', 'GET', INVALID_CREDS, 301), + ('/private-old', 'POST', INVALID_CREDS, 308), + ('/private-old', 'PUT', INVALID_CREDS, 308), + ('/private-old', 'PATCH', INVALID_CREDS, 308), + ('/private-old', 'DELETE', INVALID_CREDS, 308), + ('/private-old', 'OPTIONS', INVALID_CREDS, 308), + ] + self.validate(expectations) + + # Same, but with following redirects, asserting on the final status + expectations = [ + ('/private-old', 'GET', INVALID_CREDS, 401), + ('/private-old', 'POST', INVALID_CREDS, 401), + ('/private-old', 'PUT', INVALID_CREDS, 401), + ('/private-old', 'PATCH', INVALID_CREDS, 401), + ('/private-old', 'DELETE', INVALID_CREDS, 401), + ('/private-old', 'OPTIONS', INVALID_CREDS, 401), + ] + self.validate(expectations, follow_redirects=True) + + def test_moved_public_dx_folder_with_creds(self): + expectations = [ + ('/public-old', 'GET', CREDS, 301), + ('/public-old', 'POST', CREDS, 308), + ('/public-old', 'PUT', CREDS, 308), + ('/public-old', 'PATCH', CREDS, 308), + ('/public-old', 'DELETE', CREDS, 308), + ('/public-old', 'OPTIONS', CREDS, 308), + ] + self.validate(expectations) + + # Same, but with following redirects, asserting on the final status + expectations = [ + ('/public-old', 'GET', CREDS, 200), + ('/public-old', 'POST', CREDS, 200), + ('/public-old', 'PUT', CREDS, 200), + ('/public-old', 'PATCH', CREDS, 200), + ('/public-old', 'DELETE', CREDS, 200), + ('/public-old', 'OPTIONS', CREDS, 200), + ] + self.validate(expectations, follow_redirects=True) + + def test_moved_public_dx_folder_without_creds(self): + expectations = [ + ('/public-old', 'GET', NO_CREDS, 301), + ('/public-old', 'POST', NO_CREDS, 308), + ('/public-old', 'PUT', NO_CREDS, 308), + ('/public-old', 'PATCH', NO_CREDS, 308), + ('/public-old', 'DELETE', NO_CREDS, 308), + ('/public-old', 'OPTIONS', NO_CREDS, 308), + ] + self.validate(expectations) + + # Same, but with following redirects, asserting on the final status + expectations = [ + ('/public-old', 'GET', NO_CREDS, 200), + ('/public-old', 'POST', NO_CREDS, 401), + ('/public-old', 'PUT', NO_CREDS, 401), + ('/public-old', 'PATCH', NO_CREDS, 401), + ('/public-old', 'DELETE', NO_CREDS, 401), + ('/public-old', 'OPTIONS', NO_CREDS, 200), + ] + self.validate(expectations, follow_redirects=True) + + def test_moved_public_dx_folder_invalid_creds(self): + expectations = [ + ('/public-old', 'GET', INVALID_CREDS, 301), + ('/public-old', 'POST', INVALID_CREDS, 308), + ('/public-old', 'PUT', INVALID_CREDS, 308), + ('/public-old', 'PATCH', INVALID_CREDS, 308), + ('/public-old', 'DELETE', INVALID_CREDS, 308), + ('/public-old', 'OPTIONS', INVALID_CREDS, 308), + ] + self.validate(expectations) + + # Same, but with following redirects, asserting on the final status + expectations = [ + ('/public-old', 'GET', INVALID_CREDS, 200), + ('/public-old', 'POST', INVALID_CREDS, 401), + ('/public-old', 'PUT', INVALID_CREDS, 401), + ('/public-old', 'PATCH', INVALID_CREDS, 401), + ('/public-old', 'DELETE', INVALID_CREDS, 401), + ('/public-old', 'OPTIONS', INVALID_CREDS, 200), + ] + self.validate(expectations, follow_redirects=True) diff --git a/src/plone/rest/tests/test_redirects.py b/src/plone/rest/tests/test_redirects.py new file mode 100644 index 0000000..ca5daa3 --- /dev/null +++ b/src/plone/rest/tests/test_redirects.py @@ -0,0 +1,177 @@ +# -*- coding: utf-8 -*- +from BTrees.OOBTree import OOSet +from plone.app.redirector.interfaces import IRedirectionStorage +from plone.app.testing import setRoles +from plone.app.testing import SITE_OWNER_NAME +from plone.app.testing import SITE_OWNER_PASSWORD +from plone.app.testing import TEST_USER_ID +from plone.rest.errors import ErrorHandling +from plone.rest.testing import PLONE_REST_FUNCTIONAL_TESTING +from zope.component import queryUtility + +import requests +import transaction +import unittest + + +class TestRedirects(unittest.TestCase): + + layer = PLONE_REST_FUNCTIONAL_TESTING + + def setUp(self): + self.app = self.layer['app'] + self.portal = self.layer['portal'] + self.portal_url = self.portal.absolute_url() + setRoles(self.portal, TEST_USER_ID, ['Manager']) + + self.portal.invokeFactory('Folder', id='folder-old') + self.portal.manage_renameObject('folder-old', 'folder-new') + transaction.commit() + + def test_get_to_moved_item_causes_301_redirect(self): + response = requests.get( + self.portal_url + '/folder-old', + headers={'Accept': 'application/json'}, + auth=(SITE_OWNER_NAME, SITE_OWNER_PASSWORD), + allow_redirects=False, + ) + self.assertEqual(301, response.status_code) + self.assertEqual(self.portal_url + '/folder-new', + response.headers['Location']) + self.assertEqual(u'', response.text) + + def test_post_to_moved_item_causes_308_redirect(self): + response = requests.post( + self.portal_url + '/folder-old', + headers={'Accept': 'application/json'}, + auth=(SITE_OWNER_NAME, SITE_OWNER_PASSWORD), + allow_redirects=False, + ) + self.assertEqual(308, response.status_code) + self.assertEqual(self.portal_url + '/folder-new', + response.headers['Location']) + self.assertEqual(u'', response.text) + + def test_unauthorized_request_to_item_still_redirects_first(self): + response = requests.get( + self.portal_url + '/folder-old', + headers={'Accept': 'application/json'}, + # No auth + allow_redirects=False, + ) + + # A request to the old URL of an item where the user doesn't have + # necessary permissions will still result in a redirect + self.assertEqual(301, response.status_code) + self.assertEqual(self.portal_url + '/folder-new', + response.headers['Location']) + self.assertEqual(u'', response.text) + + # Following the redirect then leads to an item that will produce a 401 + response = requests.get( + response.headers['Location'], + headers={'Accept': 'application/json'}, + # No auth + allow_redirects=False, + ) + self.assertEqual(401, response.status_code) + + def test_query_string_gets_preserved(self): + response = requests.get( + self.portal_url + '/folder-old?key=value', + headers={'Accept': 'application/json'}, + auth=(SITE_OWNER_NAME, SITE_OWNER_PASSWORD), + allow_redirects=False, + ) + self.assertEqual(301, response.status_code) + self.assertEqual(self.portal_url + '/folder-new?key=value', + response.headers['Location']) + self.assertEqual(u'', response.text) + + def test_named_service_on_moved_item_causes_301_redirect(self): + response = requests.get( + self.portal_url + '/folder-old/namedservice', + headers={'Accept': 'application/json'}, + auth=(SITE_OWNER_NAME, SITE_OWNER_PASSWORD), + allow_redirects=False, + ) + self.assertEqual(301, response.status_code) + self.assertEqual(self.portal_url + '/folder-new/namedservice', + response.headers['Location']) + self.assertEqual(u'', response.text) + + def test_named_service_plus_path_parameter_works(self): + response = requests.get( + self.portal_url + '/folder-old/namedservice/param', + headers={'Accept': 'application/json'}, + auth=(SITE_OWNER_NAME, SITE_OWNER_PASSWORD), + allow_redirects=False, + ) + self.assertEqual(301, response.status_code) + self.assertEqual(self.portal_url + '/folder-new/namedservice/param', + response.headers['Location']) + self.assertEqual(u'', response.text) + + def test_redirects_for_regular_views_still_work(self): + response = requests.get( + self.portal_url + '/folder-old/@@some-view', + headers={'Accept': 'application/json'}, + auth=(SITE_OWNER_NAME, SITE_OWNER_PASSWORD), + allow_redirects=False, + ) + self.assertEqual(301, response.status_code) + self.assertEqual(self.portal_url + '/folder-new/@@some-view', + response.headers['Location']) + self.assertEqual(u'', response.text) + + def test_redirects_for_views_plus_params_plus_querystring_works(self): + response = requests.get( + self.portal_url + '/folder-old/@@some-view/param?k=v', + headers={'Accept': 'application/json'}, + auth=(SITE_OWNER_NAME, SITE_OWNER_PASSWORD), + allow_redirects=False, + ) + self.assertEqual(301, response.status_code) + self.assertEqual(self.portal_url + '/folder-new/@@some-view/param?k=v', + response.headers['Location']) + self.assertEqual(u'', response.text) + + def test_doesnt_cause_redirect_loop_on_bogus_storage_entries(self): + storage = queryUtility(IRedirectionStorage) + storage._paths['/plone/same'] = '/plone/same' + storage._rpaths['/plone/same'] = OOSet(['/plone/same']) + transaction.commit() + + response = requests.get( + self.portal_url + '/same/@@view', + headers={'Accept': 'application/json'}, + auth=(SITE_OWNER_NAME, SITE_OWNER_PASSWORD), + allow_redirects=False, + ) + self.assertEqual(404, response.status_code) + + def test_handles_redirects_that_include_querystring_in_old_path(self): + storage = queryUtility(IRedirectionStorage) + storage.add('/plone/some-item?key=value', '/plone/new-item') + transaction.commit() + + response = requests.get( + self.portal_url + '/some-item?key=value', + headers={'Accept': 'application/json'}, + auth=(SITE_OWNER_NAME, SITE_OWNER_PASSWORD), + allow_redirects=False, + ) + self.assertEqual(301, response.status_code) + self.assertEqual(self.portal_url + '/new-item', + response.headers['Location']) + self.assertEqual(u'', response.text) + + def test_aborts_redirect_checks_early_for_app_root(self): + error_view = ErrorHandling(self.portal, self.portal.REQUEST) + self.assertIsNone( + error_view.find_redirect_if_view_or_service([''], None)) + + def test_gracefully_deals_with_missing_request_url(self): + error_view = ErrorHandling(self.portal, self.portal.REQUEST) + self.portal.REQUEST['ACTUAL_URL'] = None + self.assertEquals(False, error_view.attempt_redirect())