Repository: jupyterhub/jupyterhub
Pull request: Handle pre_spawn_start possible exceptions
This should fix https://github.com/jupyterhub/jupyterhub/issues/2683 :eyes: 


Problem statement:
pre_spawn_start exceptions may lead to redirect loop
**Describe the bug**

if we override authenticator `pre_spawn_start` with a coroutine and throw exceptions from it, it could lead to the redirect loop issue with an eventual 500 error, instead of showing the exception.  however, exceptions `options_from_form` are rendered properly as error message.

**To Reproduce**
Steps to reproduce the behavior:
1. override authenticator pre_spawn_start and throw exception from it
2. try to launch user server from hub
3. gets infinite redirect
4. reload and see 500 error

**Expected behavior**

errors are displayed in the spawner page


cc/@aar0nTw

Hints from issue discussion:

Commit message: Address feedback

Patch to review:
diff --git a/jupyterhub/handlers/pages.py b/jupyterhub/handlers/pages.py
index 2aa2c3e846..53525dc8fa 100644
--- a/jupyterhub/handlers/pages.py
+++ b/jupyterhub/handlers/pages.py
@@ -172,7 +172,17 @@ async def get(self, for_user=None, server_name=''):
                 spawner._spawn_future = None
             # not running, no form. Trigger spawn and redirect back to /user/:name
             f = asyncio.ensure_future(self.spawn_single_user(user, server_name))
-            await asyncio.wait([f], timeout=1)
+            done, pending = await asyncio.wait([f], timeout=1)
+            # If spawn_single_user throws an exception, raise a 500 error
+            # otherwise it may cause a redirect loop
+            if f.done() and f.exception():
+                exc = f.exception()
+                if exc:
+                    raise web.HTTPError(
+                        500,
+                        "Error in Authenticator.pre_spawn_start: %s %s"
+                        % (type(exc).__name__, str(exc)),
+                    )
             self.redirect(pending_url)
 
     @web.authenticated
diff --git a/jupyterhub/tests/test_pages.py b/jupyterhub/tests/test_pages.py
index 6bc5663dac..77b5cf1ec2 100644
--- a/jupyterhub/tests/test_pages.py
+++ b/jupyterhub/tests/test_pages.py
@@ -790,3 +790,42 @@ async def test_metrics_auth(app):
 async def test_health_check_request(app):
     r = await get_page('health', app)
     assert r.status_code == 200
+
+
+async def test_pre_spawn_start_exc_no_form(app):
+    exc = "pre_spawn_start error"
+
+    # throw exception from pre_spawn_start
+    @gen.coroutine
+    def mock_pre_spawn_start(user, spawner):
+        raise Exception(exc)
+
+    with mock.patch.object(app.authenticator, 'pre_spawn_start', mock_pre_spawn_start):
+        cookies = await app.login_user('summer')
+        # spawn page should thow a 500 error and show the pre_spawn_start error message
+        r = await get_page('spawn', app, cookies=cookies)
+        assert r.status_code == 500
+        assert exc in r.text
+
+
+async def test_pre_spawn_start_exc_options_form(app):
+    exc = "pre_spawn_start error"
+
+    # throw exception from pre_spawn_start
+    @gen.coroutine
+    def mock_pre_spawn_start(user, spawner):
+        raise Exception(exc)
+
+    with mock.patch.dict(
+        app.users.settings, {'spawner_class': FormSpawner}
+    ), mock.patch.object(app.authenticator, 'pre_spawn_start', mock_pre_spawn_start):
+        cookies = await app.login_user('spring')
+        user = app.users['spring']
+        # spawn page shouldn't throw any error until the spawn is started
+        r = await get_page('spawn', app, cookies=cookies)
+        assert r.url.endswith('/spawn')
+        r.raise_for_status()
+        assert FormSpawner.options_form in r.text
+        # spawning the user server should throw the pre_spawn_start error
+        with pytest.raises(Exception, match="%s" % exc):
+            await user.spawn()
diff --git a/jupyterhub/user.py b/jupyterhub/user.py
index 97a3543fde..407310dbcf 100644
--- a/jupyterhub/user.py
+++ b/jupyterhub/user.py
@@ -529,17 +529,19 @@ async def spawn(self, server_name='', options=None, handler=None):
 
         # trigger pre-spawn hook on authenticator
         authenticator = self.authenticator
-        if authenticator:
-            await maybe_future(authenticator.pre_spawn_start(self, spawner))
-
-        spawner._start_pending = True
-        # update spawner start time, and activity for both spawner and user
-        self.last_activity = (
-            spawner.orm_spawner.started
-        ) = spawner.orm_spawner.last_activity = datetime.utcnow()
-        db.commit()
-        # wait for spawner.start to return
         try:
+            if authenticator:
+                # pre_spawn_start can thow errors that can lead to a redirect loop
+                # if left uncaught (see https://github.com/jupyterhub/jupyterhub/issues/2683)
+                await maybe_future(authenticator.pre_spawn_start(self, spawner))
+
+            spawner._start_pending = True
+            # update spawner start time, and activity for both spawner and user
+            self.last_activity = (
+                spawner.orm_spawner.started
+            ) = spawner.orm_spawner.last_activity = datetime.utcnow()
+            db.commit()
+            # wait for spawner.start to return
             # run optional preparation work to bootstrap the notebook
             await maybe_future(spawner.run_pre_spawn_hook())
             if self.settings.get('internal_ssl'):


Code review comments:
diff_hunk: @@ -172,7 +172,17 @@ def _render_form(self, for_user, spawner_options_form, message=''):
                 spawner._spawn_future = None
             # not running, no form. Trigger spawn and redirect back to /user/:name
             f = asyncio.ensure_future(self.spawn_single_user(user, server_name))
-            await asyncio.wait([f], timeout=1)
+            done, pending = await asyncio.wait([f], timeout=1)
+            # If spawn_single_user throws an exception, raise a 500 error
+            # otherwise it may cause a redirect loop
+            if f.done() and f.exception():
+                exc = f.exception()
+                if exc:
path: jupyterhub/handlers/pages.py
text: @user1:
This `if exc` is no longer needed, since we know it's true from above.

@author:
ups... sorry, I missed it :disappointed: