From 62b991649bc8d7aef5443ae0d7be0d5e6c15b585 Mon Sep 17 00:00:00 2001 From: Will Starms Date: Fri, 26 Oct 2018 17:28:58 -0500 Subject: [PATCH 1/3] Share authenticated dict with auth functions Adds a compatibility fix to be removed at a future date for the check_x functions. --- jupyterhub/app.py | 4 ++-- jupyterhub/auth.py | 35 +++++++++++++++++++++++++++++------ jupyterhub/tests/test_auth.py | 18 ++++++++++++++++++ 3 files changed, 49 insertions(+), 8 deletions(-) diff --git a/jupyterhub/app.py b/jupyterhub/app.py index 036fb6de..e0cfa4dc 100644 --- a/jupyterhub/app.py +++ b/jupyterhub/app.py @@ -1490,7 +1490,7 @@ class JupyterHub(Application): db.add(group) for username in usernames: username = self.authenticator.normalize_username(username) - if not (await maybe_future(self.authenticator.check_whitelist(username))): + if not (await maybe_future(self.authenticator.check_whitelist(username, None))): raise ValueError("Username %r is not in whitelist" % username) user = orm.User.find(db, name=username) if user is None: @@ -1514,7 +1514,7 @@ class JupyterHub(Application): for token, name in token_dict.items(): if kind == 'user': name = self.authenticator.normalize_username(name) - if not (await maybe_future(self.authenticator.check_whitelist(name))): + if not (await maybe_future(self.authenticator.check_whitelist(name, None))): raise ValueError("Token name %r is not in whitelist" % name) if not self.authenticator.validate_username(name): raise ValueError("Token name %r is not valid" % name) diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index 57aa412c..9d0eb570 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -4,11 +4,13 @@ # Distributed under the terms of the Modified BSD License. from concurrent.futures import ThreadPoolExecutor +import inspect import pipes import re from shutil import which import sys from subprocess import Popen, PIPE, STDOUT +import warnings try: import pamela @@ -222,20 +224,25 @@ class Authenticator(LoggingConfigurable): username = self.username_map.get(username, username) return username - def check_whitelist(self, username): + # FIXME: Remove None default on removal of old signature compatibility + def check_whitelist(self, username, authentication=None): """Check if a username is allowed to authenticate based on whitelist configuration Return True if username is allowed, False otherwise. No whitelist means any username is allowed. Names are normalized *before* being checked against the whitelist. + + .. versionchanged:: 1.0 + Signature updated to accept authentication data and any future changes """ if not self.whitelist: # No whitelist means any name is allowed return True return username in self.whitelist - def check_blacklist(self, username): + # FIXME: Remove None default on removal of old signature compatibility + def check_blacklist(self, username, authentication=None): """Check if a username is blocked to authenticate based on blacklist configuration Return True if username is allowed, False otherwise. @@ -244,6 +251,9 @@ class Authenticator(LoggingConfigurable): Names are normalized *before* being checked against the blacklist. .. versionadded: 0.9 + + .. versionchanged:: 1.0 + Signature updated to accept authentication data and any future changes """ if not self.blacklist: # No blacklist means any name is allowed @@ -290,14 +300,26 @@ class Authenticator(LoggingConfigurable): self.log.warning("Disallowing invalid username %r.", username) return - blacklist_pass = await maybe_future(self.check_blacklist(username)) - whitelist_pass = await maybe_future(self.check_whitelist(username)) + if len(inspect.getfullargspec(self.check_blacklist).args) != 3: + self.log.warning("DEPRECATION WARNING: check_xlist auth functions are changing signature, refer to docs to prevent future failures.") + warnings.warn("check_xlist auth functions are changing signature, refer to docs to prevent future failures", DeprecationWarning) + blacklist_pass = await maybe_future(self.check_blacklist(username)) + else: + blacklist_pass = await maybe_future(self.check_blacklist(username, authenticated)) + if blacklist_pass: pass else: self.log.warning("User %r in blacklist. Stop authentication", username) return + if len(inspect.getfullargspec(self.check_whitelist).args) != 3: + self.log.warning("DEPRECATION WARNING: check_xlist auth functions are changing signature, refer to docs to prevent future failures.") + warnings.warn("check_xlist auth functions are changing signature, refer to docs to prevent future failures", DeprecationWarning) + whitelist_pass = await maybe_future(self.check_whitelist(username)) + else: + whitelist_pass = await maybe_future(self.check_whitelist(username, authenticated)) + if whitelist_pass: if authenticated['admin'] is None: authenticated['admin'] = await maybe_future(self.is_admin(handler, authenticated)) @@ -557,11 +579,12 @@ class LocalAuthenticator(Authenticator): "Ignoring username whitelist because group whitelist supplied!" ) - def check_whitelist(self, username): + # FIXME: Remove None default on removal of old signature comptability + def check_whitelist(self, username, authentication=None): if self.group_whitelist: return self.check_group_whitelist(username) else: - return super().check_whitelist(username) + return super().check_whitelist(username, authentication) def check_group_whitelist(self, username): """ diff --git a/jupyterhub/tests/test_auth.py b/jupyterhub/tests/test_auth.py index 655a980e..ea4005e0 100644 --- a/jupyterhub/tests/test_auth.py +++ b/jupyterhub/tests/test_auth.py @@ -237,6 +237,24 @@ def test_pam_auth_blacklist(): assert authorized['name'] == 'kaylee' +@pytest.mark.gen_test +def test_deprecated_signatures(): + deprecated_authenticator = MockPAMAuthenticator() + + def deprecated_xlist(username): + return True + + with mock.patch.multiple(deprecated_authenticator, + check_whitelist=deprecated_xlist, + check_blacklist=deprecated_xlist): + authorized = yield deprecated_authenticator.get_authenticated_user(None, { + 'username': 'test', + 'password': 'test' + }) + + assert authorized is not None + + @pytest.mark.gen_test def test_pam_auth_no_such_group(): authenticator = MockPAMAuthenticator(group_whitelist={'nosuchcrazygroup'}) From bb83bb47d8f1eeae6e1202ba5e83f6ccb469ae0b Mon Sep 17 00:00:00 2001 From: Min RK Date: Fri, 15 Feb 2019 13:20:20 +0100 Subject: [PATCH 2/3] auth: apply adaptation to deprecated signature in init applies/warns in init, ensures compatibility with a wrapper rather than warning/calling differently at call time, which won't take effect everywhere --- jupyterhub/auth.py | 55 ++++++++++++++++++++++------------- jupyterhub/tests/test_auth.py | 13 +++++---- 2 files changed, 43 insertions(+), 25 deletions(-) diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index 5152e97a..2b9230e2 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -216,6 +216,35 @@ class Authenticator(LoggingConfigurable): """ ) + def __init__(self, **kwargs): + super().__init__(**kwargs) + for method_name in ('check_whitelist', 'check_blacklist', 'check_group_whitelist'): + original_method = getattr(self, method_name) + signature = inspect.signature(original_method) + if 'authentication' not in signature.parameters: + # adapt to pre-1.0 signature for compatibility + warnings.warn( + """ + {0}.{1} does not support the authentication argument, + added in JupyterHub 1.0. + + It should have the signature: + + def {1}(self, username, authentication=None): + ... + + Adapting for compatibility. + """.format( + self.__class__.__name__, + method_name, + ), + DeprecationWarning + ) + + def wrapped_method(username, authentication=None, **kwargs): + return original_method(username, **kwargs) + setattr(self, method_name, wrapped_method) + def normalize_username(self, username): """Normalize the given username and return it @@ -228,7 +257,6 @@ class Authenticator(LoggingConfigurable): username = self.username_map.get(username, username) return username - # FIXME: Remove None default on removal of old signature compatibility def check_whitelist(self, username, authentication=None): """Check if a username is allowed to authenticate based on whitelist configuration @@ -245,7 +273,6 @@ class Authenticator(LoggingConfigurable): return True return username in self.whitelist - # FIXME: Remove None default on removal of old signature compatibility def check_blacklist(self, username, authentication=None): """Check if a username is blocked to authenticate based on blacklist configuration @@ -257,7 +284,7 @@ class Authenticator(LoggingConfigurable): .. versionadded: 0.9 .. versionchanged:: 1.0 - Signature updated to accept authentication data and any future changes + Signature updated to accept authentication data as second argument """ if not self.blacklist: # No blacklist means any name is allowed @@ -304,12 +331,8 @@ class Authenticator(LoggingConfigurable): self.log.warning("Disallowing invalid username %r.", username) return - if len(inspect.getfullargspec(self.check_blacklist).args) != 3: - self.log.warning("DEPRECATION WARNING: check_xlist auth functions are changing signature, refer to docs to prevent future failures.") - warnings.warn("check_xlist auth functions are changing signature, refer to docs to prevent future failures", DeprecationWarning) - blacklist_pass = await maybe_future(self.check_blacklist(username)) - else: - blacklist_pass = await maybe_future(self.check_blacklist(username, authenticated)) + blacklist_pass = await maybe_future(self.check_blacklist(username, authenticated)) + whitelist_pass = await maybe_future(self.check_whitelist(username, authenticated)) if blacklist_pass: pass @@ -317,13 +340,6 @@ class Authenticator(LoggingConfigurable): self.log.warning("User %r in blacklist. Stop authentication", username) return - if len(inspect.getfullargspec(self.check_whitelist).args) != 3: - self.log.warning("DEPRECATION WARNING: check_xlist auth functions are changing signature, refer to docs to prevent future failures.") - warnings.warn("check_xlist auth functions are changing signature, refer to docs to prevent future failures", DeprecationWarning) - whitelist_pass = await maybe_future(self.check_whitelist(username)) - else: - whitelist_pass = await maybe_future(self.check_whitelist(username, authenticated)) - if whitelist_pass: if authenticated['admin'] is None: authenticated['admin'] = await maybe_future(self.is_admin(handler, authenticated)) @@ -373,7 +389,7 @@ class Authenticator(LoggingConfigurable): authentication: The authetication dict generated by `authenticate`. Returns: admin_status (Bool or None): - The admin status of the user, or None if it could not be + The admin status of the user, or None if it could not be determined or should not change. """ return True if authentication['name'] in self.admin_users else None @@ -584,14 +600,13 @@ class LocalAuthenticator(Authenticator): "Ignoring username whitelist because group whitelist supplied!" ) - # FIXME: Remove None default on removal of old signature comptability def check_whitelist(self, username, authentication=None): if self.group_whitelist: - return self.check_group_whitelist(username) + return self.check_group_whitelist(username, authentication) else: return super().check_whitelist(username, authentication) - def check_group_whitelist(self, username): + def check_group_whitelist(self, username, authentication=None): """ If group_whitelist is configured, check if authenticating user is part of group. """ diff --git a/jupyterhub/tests/test_auth.py b/jupyterhub/tests/test_auth.py index f5055af3..930c7ab0 100644 --- a/jupyterhub/tests/test_auth.py +++ b/jupyterhub/tests/test_auth.py @@ -233,14 +233,17 @@ async def test_pam_auth_blacklist(): async def test_deprecated_signatures(): - deprecated_authenticator = MockPAMAuthenticator() - def deprecated_xlist(username): + def deprecated_xlist(self, username): return True - with mock.patch.multiple(deprecated_authenticator, - check_whitelist=deprecated_xlist, - check_blacklist=deprecated_xlist): + with mock.patch.multiple( + MockPAMAuthenticator, + check_whitelist=deprecated_xlist, + check_group_whitelist=deprecated_xlist, + check_blacklist=deprecated_xlist, + ): + deprecated_authenticator = MockPAMAuthenticator() authorized = await deprecated_authenticator.get_authenticated_user(None, { 'username': 'test', 'password': 'test' From 701b93d2266ae8e3bc7cbf06a3aaed94822aae29 Mon Sep 17 00:00:00 2001 From: Min RK Date: Fri, 15 Feb 2019 15:09:56 +0100 Subject: [PATCH 3/3] check_group_whitelist is optional --- jupyterhub/auth.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/jupyterhub/auth.py b/jupyterhub/auth.py index 2b9230e2..1816e85e 100644 --- a/jupyterhub/auth.py +++ b/jupyterhub/auth.py @@ -219,7 +219,10 @@ class Authenticator(LoggingConfigurable): def __init__(self, **kwargs): super().__init__(**kwargs) for method_name in ('check_whitelist', 'check_blacklist', 'check_group_whitelist'): - original_method = getattr(self, method_name) + original_method = getattr(self, method_name, None) + if original_method is None: + # no such method (check_group_whitelist is optional) + continue signature = inspect.signature(original_method) if 'authentication' not in signature.parameters: # adapt to pre-1.0 signature for compatibility