diff --git a/jupyterhub/handlers/base.py b/jupyterhub/handlers/base.py index 0102eb83..e234b3aa 100644 --- a/jupyterhub/handlers/base.py +++ b/jupyterhub/handlers/base.py @@ -236,7 +236,7 @@ class BaseHandler(RequestHandler): self.db.commit() return self._user_from_orm(orm_token.user) - async def refresh_user_auth(self, user, force=False): + async def refresh_auth(self, user, force=False): """Refresh user authentication info Calls `authenticator.refresh_user(user)` @@ -257,7 +257,6 @@ class BaseHandler(RequestHandler): if not force and user._auth_refreshed and (now - user._auth_refreshed < refresh_age): # auth up-to-date return user - user._auth_refreshed = now # refresh a user at most once per request if not hasattr(self, '_refreshed_users'): @@ -277,6 +276,8 @@ class BaseHandler(RequestHandler): ) return + user._auth_refreshed = now + if auth_info == True: # refresh_user confirmed that it's up-to-date, # nothing to refresh @@ -358,7 +359,7 @@ class BaseHandler(RequestHandler): if user is None: user = self.get_current_user_cookie() if user: - user = await self.refresh_user_auth(user) + user = await self.refresh_auth(user) self._jupyterhub_user = user except Exception: # don't let errors here raise more than once @@ -612,6 +613,7 @@ class BaseHandler(RequestHandler): self.statsd.incr('login.success') self.statsd.timing('login.authenticate.success', auth_timer.ms) self.log.info("User logged in: %s", user.name) + user._auth_refreshed = time.monotonic() return user else: self.statsd.incr('login.failure') @@ -646,9 +648,9 @@ class BaseHandler(RequestHandler): async def spawn_single_user(self, user, server_name='', options=None): # in case of error, include 'try again from /hub/home' message if self.authenticator.refresh_pre_spawn: - auth_user = await self.refresh_user(user, force=True) + auth_user = await self.refresh_auth(user, force=True) if auth_user is None: - raise web.HTTPError(403, "auth has expired for %s, login again", auth_user.name) + raise web.HTTPError(403, "auth has expired for %s, login again", user.name) spawn_start_time = time.perf_counter() self.extra_error_html = self.spawn_home_error diff --git a/jupyterhub/tests/test_auth_expiry.py b/jupyterhub/tests/test_auth_expiry.py new file mode 100644 index 00000000..278b9169 --- /dev/null +++ b/jupyterhub/tests/test_auth_expiry.py @@ -0,0 +1,179 @@ +""" +test authentication expiry + +authentication can expire in a number of ways: + +- needs refresh and can be refreshed +- doesn't need refresh +- needs refresh and cannot be refreshed without new login +""" + +import asyncio +from contextlib import contextmanager +from unittest import mock +from urllib.parse import parse_qs, urlparse + +import pytest + +from .utils import api_request, get_page + + +async def refresh_expired(authenticator, user): + return None + + +@pytest.fixture +def disable_refresh(app): + """Fixture disabling auth refresh""" + with mock.patch.object(app.authenticator, 'refresh_user', refresh_expired): + yield + + +@pytest.fixture +def refresh_pre_spawn(app): + """Fixture enabling auth refresh pre spawn""" + app.authenticator.refresh_pre_spawn = True + try: + yield + finally: + app.authenticator.refresh_pre_spawn = False + + +async def test_auth_refresh_at_login(app, user): + # auth_refreshed starts unset: + assert not user._auth_refreshed + # login sets auth_refreshed timestamp + await app.login_user(user.name) + assert user._auth_refreshed + user._auth_refreshed -= 10 + before = user._auth_refreshed + # login again updates auth_refreshed timestamp + # even when auth is fresh + await app.login_user(user.name) + assert user._auth_refreshed > before + + +async def test_auth_refresh_page(app, user): + cookies = await app.login_user(user.name) + assert user._auth_refreshed + user._auth_refreshed -= 10 + before = user._auth_refreshed + + # get a page with auth not expired + # doesn't trigger refresh + r = await get_page('home', app, cookies=cookies) + assert r.status_code == 200 + assert user._auth_refreshed == before + + # get a page with stale auth, refreshes auth + user._auth_refreshed -= app.authenticator.auth_refresh_age + r = await get_page('home', app, cookies=cookies) + assert r.status_code == 200 + assert user._auth_refreshed > before + + +async def test_auth_expired_page(app, user, disable_refresh): + cookies = await app.login_user(user.name) + assert user._auth_refreshed + user._auth_refreshed -= 10 + before = user._auth_refreshed + + # auth is fresh, doesn't trigger expiry + r = await get_page('home', app, cookies=cookies) + assert user._auth_refreshed == before + assert r.status_code == 200 + + # get a page with stale auth, triggers expiry + user._auth_refreshed -= app.authenticator.auth_refresh_age + before = user._auth_refreshed + r = await get_page('home', app, cookies=cookies, allow_redirects=False) + + # verify that we redirect to login with ?next=requested page + assert r.status_code == 302 + redirect_url = urlparse(r.headers['Location']) + assert redirect_url.path.endswith('/login') + query = parse_qs(redirect_url.query) + assert query['next'] + next_url = urlparse(query['next'][0]) + assert next_url.path == urlparse(r.url).path + + # make sure refresh didn't get updated + assert user._auth_refreshed == before + + +async def test_auth_expired_api(app, user, disable_refresh): + cookies = await app.login_user(user.name) + assert user._auth_refreshed + user._auth_refreshed -= 10 + before = user._auth_refreshed + + # auth is fresh, doesn't trigger expiry + r = await api_request(app, 'users/' + user.name, name=user.name) + assert user._auth_refreshed == before + assert r.status_code == 200 + + # get a page with stale auth, triggers expiry + user._auth_refreshed -= app.authenticator.auth_refresh_age + r = await api_request(app, 'users/' + user.name, name=user.name) + # api requests can't do login redirects + assert r.status_code == 403 + + +async def test_refresh_pre_spawn(app, user, refresh_pre_spawn): + cookies = await app.login_user(user.name) + assert user._auth_refreshed + user._auth_refreshed -= 10 + before = user._auth_refreshed + + # auth is fresh, but should be forced to refresh by spawn + r = await api_request( + app, 'users/{}/server'.format(user.name), method='post', name=user.name + ) + assert 200 <= r.status_code < 300 + assert user._auth_refreshed > before + + +async def test_refresh_pre_spawn_expired(app, user, refresh_pre_spawn, disable_refresh): + cookies = await app.login_user(user.name) + assert user._auth_refreshed + user._auth_refreshed -= 10 + before = user._auth_refreshed + + # auth is fresh, doesn't trigger expiry + r = await api_request( + app, 'users/{}/server'.format(user.name), method='post', name=user.name + ) + assert r.status_code == 403 + assert user._auth_refreshed == before + + +async def test_refresh_pre_spawn_admin_request( + app, user, admin_user, refresh_pre_spawn +): + await app.login_user(user.name) + await app.login_user(admin_user.name) + user._auth_refreshed -= 10 + before = user._auth_refreshed + + # admin request, auth is fresh. Should still refresh user auth. + r = await api_request( + app, 'users', user.name, 'server', method='post', name=admin_user.name + ) + assert 200 <= r.status_code < 300 + assert user._auth_refreshed > before + + +async def test_refresh_pre_spawn_expired_admin_request( + app, user, admin_user, refresh_pre_spawn, disable_refresh +): + await app.login_user(user.name) + await app.login_user(admin_user.name) + user._auth_refreshed -= 10 + + # auth needs refresh but can't without a new login; spawn should fail + user._auth_refreshed -= app.authenticator.auth_refresh_age + r = await api_request( + app, 'users', user.name, 'server', method='post', name=admin_user.name + ) + # api requests can't do login redirects + assert r.status_code == 403 diff --git a/jupyterhub/tests/utils.py b/jupyterhub/tests/utils.py index e675270d..8214484d 100644 --- a/jupyterhub/tests/utils.py +++ b/jupyterhub/tests/utils.py @@ -129,7 +129,7 @@ async def api_request(app, *api_path, **kwargs): # make a copy to avoid modifying arg in-place kwargs['headers'] = h = {} h.update(headers) - h.update(auth_header(app.db, 'admin')) + h.update(auth_header(app.db, kwargs.pop('name', 'admin'))) url = ujoin(base_url, 'api', *api_path) method = kwargs.pop('method', 'get') diff --git a/jupyterhub/user.py b/jupyterhub/user.py index e467db99..5944ade5 100644 --- a/jupyterhub/user.py +++ b/jupyterhub/user.py @@ -138,7 +138,7 @@ class User: orm_user = None log = app_log settings = None - auth_refreshed = None + _auth_refreshed = None def __init__(self, orm_user, settings=None, db=None): self.db = db or inspect(orm_user).session