diff --git a/jupyterhub/services/auth.py b/jupyterhub/services/auth.py index 9ba97027..195c2cb7 100644 --- a/jupyterhub/services/auth.py +++ b/jupyterhub/services/auth.py @@ -1043,19 +1043,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 1440c59e..e05a76f9 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 92ec5286..e95a11ab 100644 --- a/jupyterhub/tests/test_singleuser.py +++ b/jupyterhub/tests/test_singleuser.py @@ -361,3 +361,16 @@ async def test_nbclassic_control_panel(app, user, full_spawn): else: prefix = app.base_url assert link["href"] == url_path_join(prefix, "hub/home") + + +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()