Repository: jupyterhub/oauthenticator
Pull request #404 (2021-01-24 11:54:19)

Clear cookie on logout
We should clear the `oauthenticator-state` cookie just in case for security.


Patch:
diff --git a/oauthenticator/globus.py b/oauthenticator/globus.py
index 51b96df..4933c5e 100644
--- a/oauthenticator/globus.py
+++ b/oauthenticator/globus.py
@@ -13,10 +13,10 @@ from tornado.httpclient import HTTPRequest
 from tornado.web import HTTPError
 from traitlets import Bool, List, Unicode, default
 
-from .oauth2 import OAuthenticator
+from .oauth2 import OAuthenticator, OAuthLogoutHandler
 
 
-class GlobusLogoutHandler(LogoutHandler):
+class GlobusLogoutHandler(OAuthLogoutHandler):
     """
     Handle custom logout URLs and token revocation. If a custom logout url
     is specified, the 'logout' button will log the user out of that identity
@@ -41,6 +41,8 @@ class GlobusLogoutHandler(LogoutHandler):
     async def handle_logout(self):
         """Overridden method for custom logout functionality. Should be called by
         Jupyterhub on logout just before destroying the users session to log them out."""
+        await super().handle_logout()
+
         if self.current_user and self.authenticator.revoke_tokens_on_logout:
             await self.clear_tokens(self.current_user)
 
@@ -245,12 +247,6 @@ class GlobusOAuthenticator(OAuthenticator):
                               )
             await self.fetch(req)
 
-    def logout_url(self, base_url):
-        return url_path_join(base_url, 'logout')
-
-    def get_handlers(self, app):
-        return super().get_handlers(app) + [(r'/logout', self.logout_handler)]
-
 
 class LocalGlobusOAuthenticator(LocalAuthenticator, GlobusOAuthenticator):
     """A version that mixes in local system user creation"""
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index 3533857..fe21741 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -11,7 +11,7 @@ import uuid
 from urllib.parse import quote, urlparse, urlunparse
 
 from jupyterhub.auth import Authenticator
-from jupyterhub.handlers import BaseHandler
+from jupyterhub.handlers import BaseHandler, LogoutHandler
 from jupyterhub.utils import url_path_join
 from tornado import web
 from tornado.auth import OAuth2Mixin
@@ -227,6 +227,11 @@ class OAuthCallbackHandler(BaseHandler):
         self.redirect(self.get_next_url(user))
 
 
+class OAuthLogoutHandler(LogoutHandler):
+    async def handle_logout(self):
+        self.clear_cookie(STATE_COOKIE_NAME)
+
+
 class OAuthenticator(Authenticator):
     """Base class for OAuthenticators
 
@@ -238,6 +243,7 @@ class OAuthenticator(Authenticator):
 
     login_handler = OAuthLoginHandler
     callback_handler = OAuthCallbackHandler
+    logout_handler = OAuthLogoutHandler
 
     authorize_url = Unicode(
         config=True, help="""The authenticate url for initiating oauth"""
@@ -371,6 +377,9 @@ class OAuthenticator(Authenticator):
     def login_url(self, base_url):
         return url_path_join(base_url, 'oauth_login')
 
+    def logout_url(self, base_url):
+        return url_path_join(base_url, 'logout')
+  
     def get_callback_url(self, handler=None):
         """Get my OAuth redirect URL
         
@@ -393,6 +402,7 @@ class OAuthenticator(Authenticator):
         return [
             (r'/oauth_login', self.login_handler),
             (r'/oauth_callback', self.callback_handler),
+            (r'/logout', self.logout_handler),
         ]
 
     async def authenticate(self, handler, data=None):


Test patch:
diff --git a/oauthenticator/tests/test_globus.py b/oauthenticator/tests/test_globus.py
index 53eac95..46a8159 100644
--- a/oauthenticator/tests/test_globus.py
+++ b/oauthenticator/tests/test_globus.py
@@ -7,7 +7,7 @@ from tornado.httpclient import HTTPResponse
 
 from unittest.mock import Mock
 
-
+from ..oauth2 import STATE_COOKIE_NAME
 from ..globus import GlobusOAuthenticator, GlobusLogoutHandler
 
 from .mocks import setup_oauth_mock, mock_handler
@@ -225,6 +225,7 @@ async def test_custom_logout(monkeypatch, mock_globus_user):
                                   authenticator=authenticator)
     monkeypatch.setattr(web.RequestHandler, 'redirect', Mock())
     logout_handler.clear_login_cookie = Mock()
+    logout_handler.clear_cookie = Mock()
     logout_handler.get_current_user = Mock(return_value=mock_globus_user)
     logout_handler._jupyterhub_user = mock_globus_user
     monkeypatch.setitem(logout_handler.settings, 'statsd', Mock())
@@ -239,6 +240,7 @@ async def test_custom_logout(monkeypatch, mock_globus_user):
     await logout_handler.get()
     logout_handler.redirect.assert_called_once_with(custom_logout_url)
     assert logout_handler.clear_login_cookie.called
+    logout_handler.clear_cookie.assert_called_once_with(STATE_COOKIE_NAME)
 
 
 async def test_logout_revokes_tokens(globus_client, monkeypatch, mock_globus_user):
diff --git a/oauthenticator/tests/test_oauth2.py b/oauthenticator/tests/test_oauth2.py
index 1e2616c..eef324f 100644
--- a/oauthenticator/tests/test_oauth2.py
+++ b/oauthenticator/tests/test_oauth2.py
@@ -1,6 +1,9 @@
 import uuid
 
-from ..oauth2 import _serialize_state, _deserialize_state
+from unittest.mock import Mock
+
+from ..oauth2 import _serialize_state, _deserialize_state, OAuthenticator, OAuthLogoutHandler, STATE_COOKIE_NAME
+from .mocks import mock_handler
 
 def test_serialize_state():
     state1 = {
@@ -11,3 +14,24 @@ def test_serialize_state():
     assert isinstance(b64_state, str)
     state2 = _deserialize_state(b64_state)
     assert state2 == state1
+
+
+async def test_custom_logout(monkeypatch):
+    login_url = "http://myhost/login"
+    authenticator = OAuthenticator()
+    logout_handler = mock_handler(OAuthLogoutHandler,
+                                  authenticator=authenticator,
+                                  login_url=login_url)
+    logout_handler.clear_login_cookie = Mock()
+    logout_handler.clear_cookie = Mock()
+    logout_handler._jupyterhub_user = Mock()
+    monkeypatch.setitem(logout_handler.settings, 'statsd', Mock())
+
+    # Sanity check: Ensure the logout handler and url are set on the hub
+    handlers = [handler for _, handler in authenticator.get_handlers(None)]
+    assert any([h == OAuthLogoutHandler for h in handlers])
+    assert authenticator.logout_url('http://myhost') == 'http://myhost/logout'
+
+    await logout_handler.get()
+    assert logout_handler.clear_login_cookie.called
+    logout_handler.clear_cookie.assert_called_once_with(STATE_COOKIE_NAME)