diff --git a/jupyterhub/services/auth.py b/jupyterhub/services/auth.py index 389c7870..6bc57fc5 100644 --- a/jupyterhub/services/auth.py +++ b/jupyterhub/services/auth.py @@ -1079,19 +1079,30 @@ class HubAuthenticated: def hub_auth(self, auth): self._hub_auth = auth + _hub_login_url = None + def get_login_url(self): """Return the Hub's login URL""" - login_url = self.hub_auth.login_url - if isinstance(self.hub_auth, HubOAuth): - # add state argument to OAuth url - state = self.hub_auth.set_state_cookie(self, next_url=self.request.uri) - login_url = url_concat(login_url, {'state': state}) + if self._hub_login_url is not None: + # cached value, don't call this more than once per handler + return self._hub_login_url # temporary override at setting level, # to allow any subclass overrides of get_login_url to preserve their effect # for example, APIHandler raises 403 to prevent redirects - with mock.patch.dict(self.application.settings, {"login_url": login_url}): - app_log.debug("Redirecting to login url: %s", login_url) - return super().get_login_url() + with mock.patch.dict( + self.application.settings, {"login_url": self.hub_auth.login_url} + ): + login_url = super().get_login_url() + app_log.debug("Redirecting to login url: %s", login_url) + + if isinstance(self.hub_auth, HubOAuth): + # add state argument to OAuth url + # must do this _after_ allowing get_login_url to raise + # so we don't set unused cookies + state = self.hub_auth.set_state_cookie(self, next_url=self.request.uri) + login_url = url_concat(login_url, {'state': state}) + self._hub_login_url = login_url + return login_url def check_hub_user(self, model): """Check whether Hub-authenticated user or service should be allowed. diff --git a/jupyterhub/singleuser/extension.py b/jupyterhub/singleuser/extension.py index 14f8870b..6929eb19 100644 --- a/jupyterhub/singleuser/extension.py +++ b/jupyterhub/singleuser/extension.py @@ -130,22 +130,30 @@ class JupyterHubIdentityProvider(IdentityProvider): def _patch_get_login_url(self, handler): original_get_login_url = handler.get_login_url + _hub_login_url = None + def get_login_url(): """Return the Hub's login URL, to begin login redirect""" - login_url = self.hub_auth.login_url - # add state argument to OAuth url - state = self.hub_auth.set_state_cookie( - handler, next_url=handler.request.uri - ) - login_url = url_concat(login_url, {'state': state}) - # temporary override at setting level, + nonlocal _hub_login_url + if _hub_login_url is not None: + # cached value, don't call this more than once per handler + return _hub_login_url + # temporary override at settings level, # to allow any subclass overrides of get_login_url to preserve their effect; # for example, APIHandler raises 403 to prevent redirects with mock.patch.dict( - handler.application.settings, {"login_url": login_url} + handler.application.settings, {"login_url": self.hub_auth.login_url} ): - self.log.debug("Redirecting to login url: %s", login_url) - return original_get_login_url() + login_url = original_get_login_url() + self.log.debug("Redirecting to login url: %s", login_url) + # add state argument to OAuth url + # must do this _after_ allowing get_login_url to raise + # so we don't set unused cookies + state = self.hub_auth.set_state_cookie( + handler, next_url=handler.request.uri + ) + _hub_login_url = url_concat(login_url, {'state': state}) + return _hub_login_url handler.get_login_url = get_login_url diff --git a/jupyterhub/tests/test_singleuser.py b/jupyterhub/tests/test_singleuser.py index fb1df18e..2b9695d4 100644 --- a/jupyterhub/tests/test_singleuser.py +++ b/jupyterhub/tests/test_singleuser.py @@ -400,3 +400,16 @@ async def test_token_url_cookie(app, user, full_spawn): assert r.status_code == 200 await user.stop() + + +async def test_api_403_no_cookie(app, user, full_spawn): + """unused oauth cookies don't get set for failed requests to API handlers""" + await user.spawn() + await app.proxy.add_user(user) + url = url_path_join(public_url(app, user), "/api/contents/") + r = await async_requests.get(url, allow_redirects=False) + # 403, not redirect + assert r.status_code == 403 + # no state cookie set + assert not r.cookies + await user.stop()