From c9688d1715e2d1d47e478e91f23894379dd7f0ca Mon Sep 17 00:00:00 2001 From: Igor Suarez-Sola Date: Thu, 2 Nov 2023 13:02:21 -0700 Subject: [PATCH 1/5] Rename DevGCDatalabAuthenticator to GCDataLabAuthenticatorNoRedirect --- dlauthenticator/__init__.py | 4 ++-- dlauthenticator/dlauthenticator.py | 10 +++++----- dlauthenticator/tests/test_bypass_file.py | 2 +- dlauthenticator/tests/test_invalid_password.py | 2 +- dlauthenticator/tests/test_invalid_user.py | 2 +- dlauthenticator/tests/test_valid_login.py | 2 +- 6 files changed, 11 insertions(+), 11 deletions(-) diff --git a/dlauthenticator/__init__.py b/dlauthenticator/__init__.py index d4ad6d0..1a95d79 100644 --- a/dlauthenticator/__init__.py +++ b/dlauthenticator/__init__.py @@ -1,3 +1,3 @@ -from dlauthenticator.dlauthenticator import BaseDataLabAuthenticator, DataLabAuthenticator, GCDataLabAuthenticator, DevGCDataLabAuthenticator +from dlauthenticator.dlauthenticator import BaseDataLabAuthenticator, DataLabAuthenticator, GCDataLabAuthenticator, GCDataLabAuthenticatorNoRedirect -__all__ = [BaseDataLabAuthenticator, DataLabAuthenticator, GCDataLabAuthenticator, DevGCDataLabAuthenticator] +__all__ = [BaseDataLabAuthenticator, DataLabAuthenticator, GCDataLabAuthenticator, GCDataLabAuthenticatorNoRedirect] diff --git a/dlauthenticator/dlauthenticator.py b/dlauthenticator/dlauthenticator.py index fe2f1d0..5be563a 100644 --- a/dlauthenticator/dlauthenticator.py +++ b/dlauthenticator/dlauthenticator.py @@ -165,7 +165,7 @@ def post_authenticate(self, handler, data, token): class DataLabAuthenticator(BaseDataLabAuthenticator): """ Data Lab Jupyter token authenticator. - Notice this class doesn't perform a log in proper, that happens on the datalab login + Notice this class doesn't perform a log in proper, that happens on the Data Lab login form, which sets a cookie with the login token in the browser, is that token the one that is used in the is class to authenticate the user. """ @@ -314,16 +314,16 @@ def pre_spawn_start(self, user, spawner): } -class DevGCDataLabAuthenticator(GCDataLabAuthenticator): +class GCDataLabAuthenticatorNoRedirect(GCDataLabAuthenticator): """ Google Cloud development authenticator class. - This class doesn't use cookies but uses the DataLab authClient login interface instead. + This class doesn't use cookies but uses the Data Lab authClient login interface instead. The cookies "next url" works only for jupyterhub clusters that have a domain name that matches datalab.noirlab.edu, however development environments often has just the ip address. - By setting the c.JupyterHub.authenticator_class to DevGCDataLabAuthenticator the log in happens + By setting the c.JupyterHub.authenticator_class to GCDataLabAuthenticatorNoRedirect the log in happens via the jupyterhub default login form. E.g. - c.JupyterHub.authenticator_class = DevGCDataLabAuthenticator + c.JupyterHub.authenticator_class = GCDataLabAuthenticatorNoRedirect """ def __init__(self, parent=None, db=None, _deprecated_db_session=None): diff --git a/dlauthenticator/tests/test_bypass_file.py b/dlauthenticator/tests/test_bypass_file.py index 7d76a48..219d783 100644 --- a/dlauthenticator/tests/test_bypass_file.py +++ b/dlauthenticator/tests/test_bypass_file.py @@ -28,7 +28,7 @@ def mock_request_handler(headers): [(dlauthenticator.BaseDataLabAuthenticator, None), (dlauthenticator.DataLabAuthenticator, mock_handler), (dlauthenticator.GCDataLabAuthenticator, mock_handler), - (dlauthenticator.DevGCDataLabAuthenticator, mock_handler) + (dlauthenticator.GCDataLabAuthenticatorNoRedirect, mock_handler) ]) def test_bypass_file(auth_class, handler): """ Test that the debug login bypass allows a valid login diff --git a/dlauthenticator/tests/test_invalid_password.py b/dlauthenticator/tests/test_invalid_password.py index b1b3bfe..8720480 100644 --- a/dlauthenticator/tests/test_invalid_password.py +++ b/dlauthenticator/tests/test_invalid_password.py @@ -7,7 +7,7 @@ # Test the only two classes where password login makes sense @pytest.mark.parametrize("auth_class", [dlauthenticator.BaseDataLabAuthenticator, - dlauthenticator.DevGCDataLabAuthenticator]) + dlauthenticator.GCDataLabAuthenticatorNoRedirect]) def test_invalid_password(auth_class): """ Test that an invalid password fails to login the user. diff --git a/dlauthenticator/tests/test_invalid_user.py b/dlauthenticator/tests/test_invalid_user.py index 610ed7c..025e639 100644 --- a/dlauthenticator/tests/test_invalid_user.py +++ b/dlauthenticator/tests/test_invalid_user.py @@ -6,7 +6,7 @@ @pytest.mark.parametrize("auth_class", [dlauthenticator.BaseDataLabAuthenticator, - dlauthenticator.DevGCDataLabAuthenticator]) + dlauthenticator.GCDataLabAuthenticatorNoRedirect]) def test_invalid_user(auth_class): """ Test that an invalid username fails to login. diff --git a/dlauthenticator/tests/test_valid_login.py b/dlauthenticator/tests/test_valid_login.py index 0670a0f..4a46c69 100644 --- a/dlauthenticator/tests/test_valid_login.py +++ b/dlauthenticator/tests/test_valid_login.py @@ -103,7 +103,7 @@ def test_gc_valid_login(self, mock_isValidToken): self.basic_asserts_for_gc_auth(res, username, uid, gid, token) def test_dev_gc_valid_login(self): - dlauth = dlauthenticator.DevGCDataLabAuthenticator() + dlauth = dlauthenticator.GCDataLabAuthenticatorNoRedirect() res = dlauth.authenticate(None, dict(username=username, password=password)).result() From f4c983b94db55424ca45b25fbad60e91a21ddfff Mon Sep 17 00:00:00 2001 From: Igor Suarez-Sola Date: Thu, 2 Nov 2023 16:31:50 -0700 Subject: [PATCH 2/5] Add a pytest for the pre_spawn_start method --- dlauthenticator/tests/test_pre_spawn_start.py | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 dlauthenticator/tests/test_pre_spawn_start.py diff --git a/dlauthenticator/tests/test_pre_spawn_start.py b/dlauthenticator/tests/test_pre_spawn_start.py new file mode 100644 index 0000000..31c7a58 --- /dev/null +++ b/dlauthenticator/tests/test_pre_spawn_start.py @@ -0,0 +1,60 @@ +import pytest +from unittest.mock import MagicMock, patch +from tornado.concurrent import Future +from .. import dlauthenticator + +user_name = "testuser" +user_uid = "666" +user_guid = "666" +user_hash = "fake-hash" +user_token = f"{user_name}:{user_uid}:{user_guid}:{user_hash}" +user_selected_profile = "big-9-gb-ram-notebook" + + +@pytest.fixture +def mock_spawner(): + mock_user = MagicMock() + mock_user.name = user_name + + # Create a Future for the auth_state to be awaited + # In Tornado, when a coroutine is yielding a value, it + # expects that value to be a Future, or another coroutine + # So the method mock_user.get_auth_state should return a + # "Future", which eventually resolves to the dictionary + # { 'token': ..., 'uid': ..., 'guid': ... } + auth_state_future = Future() + auth_state_future.set_result({ + 'token': user_token, 'uid': user_uid, 'guid': user_guid + }) + mock_user.get_auth_state = MagicMock(return_value=auth_state_future) + + spawner = MagicMock() + spawner.user = mock_user + spawner.user_options = {'profile': user_selected_profile} + + return spawner + + +# Run the pytest on these two classes +authenticator_classes = [ + dlauthenticator.GCDataLabAuthenticator, + dlauthenticator.GCDataLabAuthenticatorNoRedirect, +] + + +@pytest.mark.asyncio +@pytest.mark.parametrize("authenticator_class", authenticator_classes) +async def test_pre_spawn_start(authenticator_class, mock_spawner): + authenticator = authenticator_class() + + with patch.object(authenticator.log, 'info') as mock_log_info: + await authenticator.pre_spawn_start(mock_spawner.user, mock_spawner) + + # Check that the log was called with the correct arguments + mock_log_info.assert_called_once_with( + f"user=[{mock_spawner.user.name}] NB=[{mock_spawner.user_options.get('profile')}]") + + # Check the spawner environment is set correctly + assert mock_spawner.environment['UPSTREAM_TOKEN'] == user_token + assert mock_spawner.environment['NB_USER'] == user_name + assert mock_spawner.environment['NB_UID'] == user_uid From bd217952d0206f0adb8c8640ae277969f4c69766 Mon Sep 17 00:00:00 2001 From: Igor Suarez-Sola Date: Thu, 2 Nov 2023 17:47:20 -0700 Subject: [PATCH 3/5] Ensure that inheriting classes receive the correct attributes. The enable_auth_state attribute should be set to True for both the GCDataLabAuthenticator and GCDataLabAuthenticatorNoRedirect classes. However, the auto_login attribute should only be set to True for the GCDataLabAuthenticator class, as this class performs token authentication with redirection. --- dlauthenticator/dlauthenticator.py | 3 ++- dlauthenticator/tests/test_pre_spawn_start.py | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/dlauthenticator/dlauthenticator.py b/dlauthenticator/dlauthenticator.py index 5be563a..b4892aa 100644 --- a/dlauthenticator/dlauthenticator.py +++ b/dlauthenticator/dlauthenticator.py @@ -327,7 +327,8 @@ class GCDataLabAuthenticatorNoRedirect(GCDataLabAuthenticator): """ def __init__(self, parent=None, db=None, _deprecated_db_session=None): - BaseDataLabAuthenticator.__init__(self, parent=parent, db=db, _deprecated_db_session=_deprecated_db_session) + super().__init__(parent, db, _deprecated_db_session) + self.auto_login = False def authenticate(self, handler, data): """ diff --git a/dlauthenticator/tests/test_pre_spawn_start.py b/dlauthenticator/tests/test_pre_spawn_start.py index 31c7a58..791bd4f 100644 --- a/dlauthenticator/tests/test_pre_spawn_start.py +++ b/dlauthenticator/tests/test_pre_spawn_start.py @@ -37,16 +37,24 @@ def mock_spawner(): # Run the pytest on these two classes authenticator_classes = [ - dlauthenticator.GCDataLabAuthenticator, - dlauthenticator.GCDataLabAuthenticatorNoRedirect, + (dlauthenticator.GCDataLabAuthenticator, {'enable_auth_state': True, + 'auto_login': True}), + (dlauthenticator.GCDataLabAuthenticatorNoRedirect, {'enable_auth_state': True, + 'auto_login': False}), ] @pytest.mark.asyncio -@pytest.mark.parametrize("authenticator_class", authenticator_classes) -async def test_pre_spawn_start(authenticator_class, mock_spawner): +@pytest.mark.parametrize("authenticator_class, expected_attrs", authenticator_classes) +async def test_pre_spawn_start(authenticator_class, expected_attrs, mock_spawner): authenticator = authenticator_class() + assert hasattr(authenticator, 'enable_auth_state') + assert authenticator.enable_auth_state == expected_attrs['enable_auth_state'] + assert hasattr(authenticator, 'auto_login') + assert authenticator.auto_login == expected_attrs['auto_login'] + + with patch.object(authenticator.log, 'info') as mock_log_info: await authenticator.pre_spawn_start(mock_spawner.user, mock_spawner) From 984c1bc8d2bb3de6e906662f66d69ad7567a60a7 Mon Sep 17 00:00:00 2001 From: Igor Suarez-Sola Date: Thu, 2 Nov 2023 18:21:07 -0700 Subject: [PATCH 4/5] improve comment on pre_spawn_start method --- dlauthenticator/dlauthenticator.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dlauthenticator/dlauthenticator.py b/dlauthenticator/dlauthenticator.py index b4892aa..f1e53b8 100644 --- a/dlauthenticator/dlauthenticator.py +++ b/dlauthenticator/dlauthenticator.py @@ -282,9 +282,9 @@ def post_authenticate(self, handler, data, token): @gen.coroutine def pre_spawn_start(self, user, spawner): # get_auth_state is a coroutine (async) wait for it with yield - # make sure the below is set in the config file - # c.Authenticator.enable_auth_state = True - # otherwise the below command will return None. + # make sure the attribute enable_auth_state = True + # is set in the class constructor, otherwise + # the below command will return None. auth_state = yield spawner.user.get_auth_state() # Dev note: From b8e2b96b463e8f6de596fe5ea6b711f056c28814 Mon Sep 17 00:00:00 2001 From: Igor Suarez-Sola Date: Wed, 15 Nov 2023 11:38:59 -0700 Subject: [PATCH 5/5] corrected typo in GCDataLabAuthenticatorNoRedirect class name --- dlauthenticator/__init__.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/dlauthenticator/__init__.py b/dlauthenticator/__init__.py index 1a95d79..64b550d 100644 --- a/dlauthenticator/__init__.py +++ b/dlauthenticator/__init__.py @@ -1,3 +1,5 @@ -from dlauthenticator.dlauthenticator import BaseDataLabAuthenticator, DataLabAuthenticator, GCDataLabAuthenticator, GCDataLabAuthenticatorNoRedirect +from dlauthenticator.dlauthenticator import BaseDataLabAuthenticator, DataLabAuthenticator +from dlauthenticator.dlauthenticator import GCDataLabAuthenticator, GCDataLabAuthenticatorNoRedirect -__all__ = [BaseDataLabAuthenticator, DataLabAuthenticator, GCDataLabAuthenticator, GCDataLabAuthenticatorNoRedirect] +__all__ = [BaseDataLabAuthenticator, DataLabAuthenticator, + GCDataLabAuthenticator, GCDataLabAuthenticatorNoRedirect]