From 391e648159c85ee48b1e76270a6ce44afb94dc02 Mon Sep 17 00:00:00 2001 From: Samuel Gaist Date: Fri, 19 Jul 2024 10:05:22 +0200 Subject: [PATCH 1/3] fix: ensure the prefix follows the same convention as the username Authenticators have a normalization function that should also be used on the prefix otherwise there might be issues when dealing with checks as the get_authenticated_user method returns a normalized user while authenticate returns the "raw" user. --- multiauthenticator/multiauthenticator.py | 3 +- .../tests/test_multiauthenticator.py | 77 +++++++++++++++---- 2 files changed, 66 insertions(+), 14 deletions(-) diff --git a/multiauthenticator/multiauthenticator.py b/multiauthenticator/multiauthenticator.py index dfe7040..721c5f8 100644 --- a/multiauthenticator/multiauthenticator.py +++ b/multiauthenticator/multiauthenticator.py @@ -81,7 +81,8 @@ class WrapperAuthenticator(URLScopeMixin, authenticator_klass): @property def username_prefix(self): - return f"{getattr(self, 'service_name', self.login_service)}{PREFIX_SEPARATOR}" + prefix = f"{getattr(self, 'service_name', self.login_service)}{PREFIX_SEPARATOR}" + return self.normalize_username(prefix) async def authenticate(self, handler, data=None, **kwargs): response = await super().authenticate(handler, data, **kwargs) diff --git a/multiauthenticator/tests/test_multiauthenticator.py b/multiauthenticator/tests/test_multiauthenticator.py index b288707..883a280 100644 --- a/multiauthenticator/tests/test_multiauthenticator.py +++ b/multiauthenticator/tests/test_multiauthenticator.py @@ -16,6 +16,11 @@ from ..multiauthenticator import MultiAuthenticator +class CustomDummyAuthenticator(DummyAuthenticator): + def normalize_username(self, username): + return username.upper() + + def test_different_authenticators(): MultiAuthenticator.authenticators = [ ( @@ -200,32 +205,37 @@ def test_username_prefix(): }, ), (PAMAuthenticator, "/pam", {"service_name": "PAM"}), + (CustomDummyAuthenticator, "/dummy", {"service_name": "Dummy"}), ] multi_authenticator = MultiAuthenticator() - assert len(multi_authenticator._authenticators) == 2 + assert len(multi_authenticator._authenticators) == 3 assert ( multi_authenticator._authenticators[0].username_prefix - == f"GitLab{PREFIX_SEPARATOR}" + == f"gitlab{PREFIX_SEPARATOR}" ) assert ( multi_authenticator._authenticators[1].username_prefix - == f"PAM{PREFIX_SEPARATOR}" + == f"pam{PREFIX_SEPARATOR}" + ) + assert ( + multi_authenticator._authenticators[2].username_prefix + == f"DUMMY{PREFIX_SEPARATOR}" ) @pytest.mark.asyncio async def test_authenticated_username_prefix(): MultiAuthenticator.authenticators = [ - (DummyAuthenticator, "/pam", {"service_name": "Dummy"}), + (CustomDummyAuthenticator, "/dummy", {"service_name": "Dummy"}), ] multi_authenticator = MultiAuthenticator() assert len(multi_authenticator._authenticators) == 1 - username = await multi_authenticator._authenticators[0].authenticate( + user = await multi_authenticator._authenticators[0].get_authenticated_user( None, {"username": "test"} ) - assert username == f"Dummy{PREFIX_SEPARATOR}test" + assert user["name"] == f"DUMMY{PREFIX_SEPARATOR}TEST" def test_username_prefix_checks(): @@ -233,29 +243,70 @@ def test_username_prefix_checks(): (PAMAuthenticator, "/pam", {"service_name": "PAM", "allowed_users": {"test"}}), ( PAMAuthenticator, - "/pam", + "/pam2", {"service_name": "PAM2", "blocked_users": {"test2"}}, ), + ( + CustomDummyAuthenticator, + "/dummy", + {"service_name": "Dummy", "allowed_users": {"TEST3"}}, + ), + ( + CustomDummyAuthenticator, + "/dummy2", + { + "service_name": "Dummy", + "allowed_users": {"TEST3"}, + "blocked_users": {"TEST4"}, + }, + ), ] multi_authenticator = MultiAuthenticator() - assert len(multi_authenticator._authenticators) == 2 + assert len(multi_authenticator._authenticators) == 4 authenticator = multi_authenticator._authenticators[0] assert authenticator.check_allowed("test") == False - assert authenticator.check_allowed("PAM:test") == True + assert authenticator.check_allowed("pam:test") == True assert ( authenticator.check_blocked_users("test") == False ) # Even if no block list, it does not have the correct prefix - assert authenticator.check_blocked_users("PAM:test") == True + assert authenticator.check_blocked_users("pam:test") == True authenticator = multi_authenticator._authenticators[1] assert authenticator.check_allowed("test2") == False assert ( - authenticator.check_allowed("PAM2:test2") == True + authenticator.check_allowed("pam2:test2") == True ) # Because allowed_users is empty - assert authenticator.check_blocked_users("test2") == False - assert authenticator.check_blocked_users("PAM2:test2") == False + assert ( + authenticator.check_blocked_users("test2") == False + ) # Because of missing prefix + assert ( + authenticator.check_blocked_users("pam2:test2") == False + ) # Because user is in blocked list + + authenticator = multi_authenticator._authenticators[2] + assert authenticator.check_allowed("TEST3") == False + assert authenticator.check_allowed("DUMMY:TEST3") == True + assert ( + authenticator.check_blocked_users("TEST3") == False + ) # Because of missing prefix + assert ( + authenticator.check_blocked_users("DUMMY:TEST3") == True + ) # Because blocked_users is empty thus allowed + + authenticator = multi_authenticator._authenticators[3] + assert authenticator.check_allowed("TEST3") == False + assert authenticator.check_allowed("DUMMY:TEST3") == True + assert ( + authenticator.check_blocked_users("TEST3") == False + ) # Because of missing prefix + assert ( + authenticator.check_blocked_users("DUMMY:TEST3") == True + ) # Because user is not in blocked list + assert ( + authenticator.check_blocked_users("DUMMY:TEST4") == False + ) # Because user is in blocked list @pytest.fixture(params=[f"test me{PREFIX_SEPARATOR}", f"second{PREFIX_SEPARATOR} test"]) From 56e94809129ba8b675456db142dca4085e762b47 Mon Sep 17 00:00:00 2001 From: Samuel Gaist Date: Fri, 19 Jul 2024 14:07:28 +0200 Subject: [PATCH 2/3] fix: adapt tests to JupyterHub version Since version 5, the logic of check_allowed has changed for a more secure version. --- multiauthenticator/tests/test_multiauthenticator.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/multiauthenticator/tests/test_multiauthenticator.py b/multiauthenticator/tests/test_multiauthenticator.py index 883a280..7788b47 100644 --- a/multiauthenticator/tests/test_multiauthenticator.py +++ b/multiauthenticator/tests/test_multiauthenticator.py @@ -2,6 +2,7 @@ # # SPDX-License-Identifier: BSD-3-Clause """Test module for the MultiAuthenticator class""" +import jupyterhub import pytest from jinja2 import Template @@ -11,6 +12,7 @@ from oauthenticator.github import GitHubOAuthenticator from oauthenticator.gitlab import GitLabOAuthenticator from oauthenticator.google import GoogleOAuthenticator +from packaging.version import Version from ..multiauthenticator import PREFIX_SEPARATOR from ..multiauthenticator import MultiAuthenticator @@ -275,9 +277,12 @@ def test_username_prefix_checks(): authenticator = multi_authenticator._authenticators[1] assert authenticator.check_allowed("test2") == False - assert ( - authenticator.check_allowed("pam2:test2") == True - ) # Because allowed_users is empty + if Version(jupyterhub.__version__) < Version("5"): + assert ( + authenticator.check_allowed("pam2:test2") == True + ) # Because allowed_users is empty + else: + assert authenticator.check_allowed("pam2:test2") == False assert ( authenticator.check_blocked_users("test2") == False ) # Because of missing prefix From 23a82cd9322ca0c0559f43f470e5c12a48a915aa Mon Sep 17 00:00:00 2001 From: Samuel Gaist Date: Fri, 19 Jul 2024 14:08:27 +0200 Subject: [PATCH 3/3] test: add a matrix entry for JupyterHub version This allows to test current and previous versions of JupyterHub. --- .github/workflows/workflow.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/workflow.yml b/.github/workflows/workflow.yml index ba1f9ea..41064ff 100644 --- a/.github/workflows/workflow.yml +++ b/.github/workflows/workflow.yml @@ -33,11 +33,14 @@ jobs: fail-fast: false matrix: python-version: - - "3.7" - "3.8" - "3.9" - "3.10" - "3.11" + jupyterhub-version: + - "3" + - "4" + - "5" steps: - uses: actions/checkout@v4 @@ -48,6 +51,7 @@ jobs: - name: Install Python dependencies run: | pip install --upgrade pip + pip install jupyterhub==${{ matrix.jupyterhub-version }} pip install -e ".[test]" - name: List packages