Merge pull request #4697 from minrk/backport-4630

Backport PR #4630: avoid setting unused oauth state cookies on API requests
This commit is contained in:
Min RK
2024-02-06 09:14:06 +01:00
committed by GitHub
3 changed files with 50 additions and 18 deletions

View File

@@ -1043,19 +1043,30 @@ class HubAuthenticated:
def hub_auth(self, auth): def hub_auth(self, auth):
self._hub_auth = auth self._hub_auth = auth
_hub_login_url = None
def get_login_url(self): def get_login_url(self):
"""Return the Hub's login URL""" """Return the Hub's login URL"""
login_url = self.hub_auth.login_url if self._hub_login_url is not None:
if isinstance(self.hub_auth, HubOAuth): # cached value, don't call this more than once per handler
# add state argument to OAuth url return self._hub_login_url
state = self.hub_auth.set_state_cookie(self, next_url=self.request.uri)
login_url = url_concat(login_url, {'state': state})
# temporary override at setting level, # temporary override at setting level,
# to allow any subclass overrides of get_login_url to preserve their effect # to allow any subclass overrides of get_login_url to preserve their effect
# for example, APIHandler raises 403 to prevent redirects # for example, APIHandler raises 403 to prevent redirects
with mock.patch.dict(self.application.settings, {"login_url": login_url}): with mock.patch.dict(
app_log.debug("Redirecting to login url: %s", login_url) self.application.settings, {"login_url": self.hub_auth.login_url}
return super().get_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): def check_hub_user(self, model):
"""Check whether Hub-authenticated user or service should be allowed. """Check whether Hub-authenticated user or service should be allowed.

View File

@@ -130,22 +130,30 @@ class JupyterHubIdentityProvider(IdentityProvider):
def _patch_get_login_url(self, handler): def _patch_get_login_url(self, handler):
original_get_login_url = handler.get_login_url original_get_login_url = handler.get_login_url
_hub_login_url = None
def get_login_url(): def get_login_url():
"""Return the Hub's login URL, to begin login redirect""" """Return the Hub's login URL, to begin login redirect"""
login_url = self.hub_auth.login_url nonlocal _hub_login_url
# add state argument to OAuth url if _hub_login_url is not None:
state = self.hub_auth.set_state_cookie( # cached value, don't call this more than once per handler
handler, next_url=handler.request.uri return _hub_login_url
) # temporary override at settings level,
login_url = url_concat(login_url, {'state': state})
# temporary override at setting level,
# to allow any subclass overrides of get_login_url to preserve their effect; # to allow any subclass overrides of get_login_url to preserve their effect;
# for example, APIHandler raises 403 to prevent redirects # for example, APIHandler raises 403 to prevent redirects
with mock.patch.dict( 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) login_url = original_get_login_url()
return 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 handler.get_login_url = get_login_url

View File

@@ -361,3 +361,16 @@ async def test_nbclassic_control_panel(app, user, full_spawn):
else: else:
prefix = app.base_url prefix = app.base_url
assert link["href"] == url_path_join(prefix, "hub/home") 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()