Merge pull request #3089 from kinow/redirect-with-parameters

jupyterhub/handlers/base.py

@@ -654,8 +654,42 @@ class BaseHandler(RequestHandler):
                     next_url = url_path_join(self.hub.base_url, 'spawn')
             else:
                 next_url = url_path_join(self.hub.base_url, 'home')
+
+        next_url = self.append_query_parameters(next_url, exclude=['next'])
         return next_url
 
+    def append_query_parameters(self, url, exclude=None):
+        """Append the current request's query parameters to the given URL.
+
+        Supports an extra optional parameter ``exclude`` that when provided must
+        contain a list of parameters to be ignored, i.e. these parameters will
+        not be added to the URL.
+
+        This is important to avoid infinite loops with the next parameter being
+        added over and over, for instance.
+
+        The default value for ``exclude`` is an array with "next". This is useful
+        as most use cases in JupyterHub (all?) won't want to include the next
+        parameter twice (the next parameter is added elsewhere to the query
+        parameters).
+
+        :param str url: a URL
+        :param list exclude: optional list of parameters to be ignored, defaults to
+        a list with "next" (to avoid redirect-loops)
+        :rtype (str)
+        """
+        if exclude is None:
+            exclude = ['next']
+        if self.request.query:
+            query_string = [
+                param
+                for param in parse_qsl(self.request.query)
+                if param[0] not in exclude
+            ]
+            if query_string:
+                url = url_concat(url, query_string)
+        return url
+
     async def auth_to_user(self, authenticated, user=None):
         """Persist data from .authenticate() or .refresh_user() to the User database
 

jupyterhub/handlers/pages.py

@@ -285,6 +285,8 @@ class SpawnHandler(BaseHandler):
             self.hub.base_url, "spawn-pending", user.escaped_name, server_name
         )
 
+        pending_url = self.append_query_parameters(pending_url, exclude=['next'])
+
         if self.get_argument('next', None):
             # preserve `?next=...` through spawn-pending
             pending_url = url_concat(pending_url, {'next': self.get_argument('next')})

jupyterhub/tests/test_pages.py

@@ -9,6 +9,7 @@ from urllib.parse import urlparse
 import pytest
 from bs4 import BeautifulSoup
 from tornado import gen
+from tornado.escape import url_escape
 from tornado.httputil import url_concat
 
 from .. import orm
@@ -516,6 +517,58 @@ async def test_user_redirect_deprecated(app, username):
     )
 
 
+@pytest.mark.parametrize(
+    'url, params, redirected_url, form_action',
+    [
+        (
+            # spawn?param=value
+            # will encode given parameters for an unauthenticated URL in the next url
+            # the next parameter will contain the app base URL (replaces BASE_URL in tests)
+            'spawn',
+            [('param', 'value')],
+            '/hub/login?next={{BASE_URL}}hub%2Fspawn%3Fparam%3Dvalue',
+            '/hub/login?next={{BASE_URL}}hub%2Fspawn%3Fparam%3Dvalue',
+        ),
+        (
+            # login?param=fromlogin&next=encoded(/hub/spawn?param=value)
+            # will drop parameters given to the login page, passing only the next url
+            'login',
+            [('param', 'fromlogin'), ('next', '/hub/spawn?param=value')],
+            '/hub/login?param=fromlogin&next=%2Fhub%2Fspawn%3Fparam%3Dvalue',
+            '/hub/login?next=%2Fhub%2Fspawn%3Fparam%3Dvalue',
+        ),
+        (
+            # login?param=value&anotherparam=anothervalue
+            # will drop parameters given to the login page, and use an empty next url
+            'login',
+            [('param', 'value'), ('anotherparam', 'anothervalue')],
+            '/hub/login?param=value&anotherparam=anothervalue',
+            '/hub/login?next=',
+        ),
+        (
+            # login
+            # simplest case, accessing the login URL, gives an empty next url
+            'login',
+            [],
+            '/hub/login',
+            '/hub/login?next=',
+        ),
+    ],
+)
+async def test_login_page(app, url, params, redirected_url, form_action):
+    url = url_concat(url, params)
+    r = await get_page(url, app)
+    redirected_url = redirected_url.replace('{{BASE_URL}}', url_escape(app.base_url))
+    assert r.url.endswith(redirected_url)
+    # now the login.html rendered template must include the given parameters in the form
+    # action URL, including the next URL
+    page = BeautifulSoup(r.text, "html.parser")
+    form = page.find("form", method="post")
+    action = form.attrs['action']
+    form_action = form_action.replace('{{BASE_URL}}', url_escape(app.base_url))
+    assert action.endswith(form_action)
+
+
 async def test_login_fail(app):
     name = 'wash'
     base_url = public_url(app)
@@ -546,26 +599,29 @@ async def test_login_strip(app):
 
 
 @pytest.mark.parametrize(
-    'running, next_url, location',
+    'running, next_url, location, params',
     [
         # default URL if next not specified, for both running and not
-        (True, '', ''),
+        (True, '', '', None),
-        (False, '', ''),
+        (False, '', '', None),
         # next_url is respected
-        (False, '/hub/admin', '/hub/admin'),
+        (False, '/hub/admin', '/hub/admin', None),
-        (False, '/user/other', '/hub/user/other'),
+        (False, '/user/other', '/hub/user/other', None),
-        (False, '/absolute', '/absolute'),
+        (False, '/absolute', '/absolute', None),
-        (False, '/has?query#andhash', '/has?query#andhash'),
+        (False, '/has?query#andhash', '/has?query#andhash', None),
         # next_url outside is not allowed
-        (False, 'relative/path', ''),
+        (False, 'relative/path', '', None),
-        (False, 'https://other.domain', ''),
+        (False, 'https://other.domain', '', None),
-        (False, 'ftp://other.domain', ''),
+        (False, 'ftp://other.domain', '', None),
-        (False, '//other.domain', ''),
+        (False, '//other.domain', '', None),
-        (False, '///other.domain/triple', ''),
+        (False, '///other.domain/triple', '', None),
-        (False, '\\\\other.domain/backslashes', ''),
+        (False, '\\\\other.domain/backslashes', '', None),
+        # params are handled correctly
+        (True, '/hub/admin', 'hub/admin?left=1&right=2', [('left', 1), ('right', 2)]),
+        (False, '/hub/admin', 'hub/admin?left=1&right=2', [('left', 1), ('right', 2)]),
     ],
 )
-async def test_login_redirect(app, running, next_url, location):
+async def test_login_redirect(app, running, next_url, location, params):
     cookies = await app.login_user('river')
     user = app.users['river']
     if location:
@@ -577,6 +633,8 @@ async def test_login_redirect(app, running, next_url, location):
         location = ujoin(app.base_url, 'hub/spawn')
 
     url = 'login'
+    if params:
+        url = url_concat(url, params)
     if next_url:
         if '//' not in next_url and next_url.startswith('/'):
             next_url = ujoin(app.base_url, next_url, '')