From 391e648159c85ee48b1e76270a6ce44afb94dc02 Mon Sep 17 00:00:00 2001 From: Samuel Gaist Date: Fri, 19 Jul 2024 10:05:22 +0200 Subject: [PATCH] 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"])