From c616913292be9066174405f9f8911212c2722af7 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 17 Sep 2024 09:20:01 +0200 Subject: [PATCH 01/11] check allowed_groups in check_allowed avoids relying on allow_all in jupyterhub 5 --- ldapauthenticator/ldapauthenticator.py | 37 +++++++++++++++---- .../tests/test_ldapauthenticator.py | 4 +- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 932316b..3f237ef 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -1,5 +1,6 @@ import enum import re +from inspect import isawaitable import ldap3 from jupyterhub.auth import Authenticator @@ -499,6 +500,7 @@ async def authenticate(self, handler, data): ) return None + ldap_groups = [] if self.allowed_groups: if not self.group_search_filter or not self.group_attributes: self.log.warning( @@ -506,7 +508,6 @@ async def authenticate(self, handler, data): ) return None self.log.debug("username:%s Using dn %s", username, userdn) - found = False for group in self.allowed_groups: found = conn.search( search_base=group, @@ -518,8 +519,11 @@ async def authenticate(self, handler, data): attributes=self.group_attributes, ) if found: - break - if not found: + ldap_groups.append(group) + # we currently only use this in check_allowed, + # so we could stop here, as only one match is relevant + # break + if not ldap_groups: # If we reach here, then none of the groups matched self.log.warning( f"username:{username} User not in any of the allowed groups" @@ -529,8 +533,25 @@ async def authenticate(self, handler, data): if not self.use_lookup_dn_username: username = data["username"] - user_info = self.get_user_attributes(conn, userdn) - if user_info: - self.log.debug("username:%s attributes:%s", username, user_info) - return {"name": username, "auth_state": user_info} - return username + user_attrs = self.get_user_attributes(conn, userdn) + auth_state = { + "ldap_groups": ldap_groups, + "user_attrs": user_attrs, + } + self.log.debug("username:%s attributes:%s", username, user_attrs) + return {"name": username, "auth_state": auth_state} + + async def check_allowed(self, username, auth_model): + allowed = super().check_allowed(username, auth_model) + if isawaitable(allowed): + allowed = allowed + if allowed is True: + return True + if self.allowed_groups: + # check allowed groups + in_groups = set((auth_model.get("auth_state") or {}).get("ldap_groups", [])) + for group in self.allowed_groups: + if group in in_groups: + self.log.debug("Allowing %s as member of group %s", username, group) + return True + return False diff --git a/ldapauthenticator/tests/test_ldapauthenticator.py b/ldapauthenticator/tests/test_ldapauthenticator.py index 87f5983..60c9358 100644 --- a/ldapauthenticator/tests/test_ldapauthenticator.py +++ b/ldapauthenticator/tests/test_ldapauthenticator.py @@ -131,7 +131,7 @@ async def test_ldap_auth_state_attributes(authenticator): None, {"username": "fry", "password": "fry"} ) assert authorized["name"] == "fry" - assert authorized["auth_state"] == {"employeeType": ["Delivery boy"]} + assert authorized["auth_state"]["user_attrs"] == {"employeeType": ["Delivery boy"]} async def test_ldap_auth_state_attributes2(authenticator): @@ -143,4 +143,4 @@ async def test_ldap_auth_state_attributes2(authenticator): None, {"username": "leela", "password": "leela"} ) assert authorized["name"] == "leela" - assert authorized["auth_state"] == {"description": ["Mutant"]} + assert authorized["auth_state"]["user_attrs"] == {"description": ["Mutant"]} From 321f0726d97c39c4488e7aaf239b88808abb75e2 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 17 Sep 2024 10:53:28 +0200 Subject: [PATCH 02/11] add missing await Co-authored-by: Erik Sundell --- ldapauthenticator/ldapauthenticator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 3f237ef..3c737f8 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -544,7 +544,7 @@ async def authenticate(self, handler, data): async def check_allowed(self, username, auth_model): allowed = super().check_allowed(username, auth_model) if isawaitable(allowed): - allowed = allowed + allowed = await allowed if allowed is True: return True if self.allowed_groups: From a5cfed7e39723a68fd80d26a5a47979f587a8773 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 17 Sep 2024 11:06:05 +0200 Subject: [PATCH 03/11] search_filter is allow config allowed_groups still narrows the match --- ldapauthenticator/ldapauthenticator.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 3c737f8..2a612f3 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -547,6 +547,11 @@ async def check_allowed(self, username, auth_model): allowed = await allowed if allowed is True: return True + if self.search_filter and not self.allowed_groups: + # search_filter was specified + # consider matching this filter sufficient to allow access + # _IF_ allowed_groups is unspecified + return True if self.allowed_groups: # check allowed groups in_groups = set((auth_model.get("auth_state") or {}).get("ldap_groups", [])) From b1b9027c90053a336b8b1a657e126b2780354274 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 17 Sep 2024 11:07:44 +0200 Subject: [PATCH 04/11] Document allowed_groups interaction with search_filter --- ldapauthenticator/ldapauthenticator.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 2a612f3..ace78b3 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -143,6 +143,9 @@ def _validate_bind_dn_template(self, proposal): Set to an empty list or None to allow all users that have an LDAP account to log in, without performing any group membership checks. + + When combined with `search_filter`, this strictly reduces the allowed users, + i.e. `search_filter` AND `allowed_groups` must both be satisfied. """, ) From 14cefb216c809a2bad706b94cf8546b25c8758d1 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 17 Sep 2024 11:09:21 +0200 Subject: [PATCH 05/11] truncate ldap_groups match --- ldapauthenticator/ldapauthenticator.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index ace78b3..02abd68 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -524,8 +524,10 @@ async def authenticate(self, handler, data): if found: ldap_groups.append(group) # we currently only use this in check_allowed, - # so we could stop here, as only one match is relevant - # break + # so we stop here, as only one match is relevant + # if all groups are needed (e.g. for manage_groups) + # we should keep fetching membership + break if not ldap_groups: # If we reach here, then none of the groups matched self.log.warning( From c199e49812aefac9fd821f163de0ac6920fe3491 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 17 Sep 2024 12:31:54 +0200 Subject: [PATCH 06/11] allowed_groups is not exclusive with allowed_users, etc. --- ldapauthenticator/ldapauthenticator.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 02abd68..a06a1b9 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -528,12 +528,6 @@ async def authenticate(self, handler, data): # if all groups are needed (e.g. for manage_groups) # we should keep fetching membership break - if not ldap_groups: - # If we reach here, then none of the groups matched - self.log.warning( - f"username:{username} User not in any of the allowed groups" - ) - return None if not self.use_lookup_dn_username: username = data["username"] @@ -552,11 +546,6 @@ async def check_allowed(self, username, auth_model): allowed = await allowed if allowed is True: return True - if self.search_filter and not self.allowed_groups: - # search_filter was specified - # consider matching this filter sufficient to allow access - # _IF_ allowed_groups is unspecified - return True if self.allowed_groups: # check allowed groups in_groups = set((auth_model.get("auth_state") or {}).get("ldap_groups", [])) From 2426145a78e9ff8be646beaf3c5256e0a8bad85d Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 17 Sep 2024 13:10:49 +0200 Subject: [PATCH 07/11] fix log.debug string format --- ldapauthenticator/ldapauthenticator.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index a06a1b9..c9343b5 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -332,10 +332,10 @@ def resolve_username(self, username_supplied_by_user): login=escape_filter_chars(username_supplied_by_user), ) self.log.debug( - "Looking up user with:\n", - f" search_base = '{self.user_search_base}'\n", - f" search_filter = '{search_filter}'\n", - f" attributes = '[{self.lookup_dn_user_dn_attribute}]'", + "Looking up user with:\n" + f" search_base = '{self.user_search_base}'\n" + f" search_filter = '{search_filter}'\n" + f" attributes = '[{self.lookup_dn_user_dn_attribute}]'" ) conn.search( search_base=self.user_search_base, From d29b64cf2bd200af349ceb92592251c308642e49 Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 17 Sep 2024 13:34:55 +0200 Subject: [PATCH 08/11] update search_filter docstring, test allowed_users --- ldapauthenticator/ldapauthenticator.py | 18 +++++++++- .../tests/test_ldapauthenticator.py | 36 +++++++++++++++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index c9343b5..34af803 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -290,7 +290,17 @@ def _validate_bind_dn_template(self, proposal): ) search_filter = Unicode( - config=True, help="LDAP3 Search Filter whose results are allowed access" + config=True, + help=""" + LDAP3 Search Filter to limit allowed users. + + Matching the search_filter is necessary but not sufficient to grant access. + Grant access by setting one or more of `allowed_users`, + `allow_all`, `allowed_groups`, etc. + + Users who do not match this filter cannot be allowed + by any other configuration. + """, ) attributes = List(config=True, help="List of attributes to be searched") @@ -553,4 +563,10 @@ async def check_allowed(self, username, auth_model): if group in in_groups: self.log.debug("Allowing %s as member of group %s", username, group) return True + if self.search_filter: + self.log.warning( + "User %s matches search_filter %s, but not allowed by allowed_users, allowed_groups, or allow_all.", + username, + self.search_filter, + ) return False diff --git a/ldapauthenticator/tests/test_ldapauthenticator.py b/ldapauthenticator/tests/test_ldapauthenticator.py index 60c9358..b875be6 100644 --- a/ldapauthenticator/tests/test_ldapauthenticator.py +++ b/ldapauthenticator/tests/test_ldapauthenticator.py @@ -107,6 +107,7 @@ async def test_ldap_auth_use_lookup_dn(authenticator): async def test_ldap_auth_search_filter(authenticator): authenticator.allowed_groups = [] + authenticator.allow_all = True authenticator.search_filter = ( "(&(objectClass=inetOrgPerson)(ou= Delivering Crew)(cn={username}))" ) @@ -115,6 +116,7 @@ async def test_ldap_auth_search_filter(authenticator): authorized = await authenticator.get_authenticated_user( None, {"username": "fry", "password": "fry"} ) + assert authorized is not None assert authorized["name"] == "fry" # proper username and password but not in search filter @@ -123,6 +125,40 @@ async def test_ldap_auth_search_filter(authenticator): ) assert authorized is None +async def test_allow_config(authenticator): + # test various sources of allow config + + # this group allows fry, leela, bender + authenticator.allowed_groups = ["cn=ship_crew,ou=people,dc=planetexpress,dc=com"] + authenticator.allowed_users = {"zoidberg"} + + # in allowed_groups + authorized = await authenticator.get_authenticated_user( + None, {"username": "fry", "password": "fry"} + ) + assert authorized is not None + assert authorized["name"] == "fry" + + # in allowed_users + authorized = await authenticator.get_authenticated_user( + None, {"username": "zoidberg", "password": "zoidberg"} + ) + assert authorized is not None + assert authorized["name"] == "zoidberg" + + # no match + authorized = await authenticator.get_authenticated_user( + None, {"username": "professor", "password": "professor"} + ) + assert authorized is None + # allow_all grants access + authenticator.allow_all = True + authorized = await authenticator.get_authenticated_user( + None, {"username": "professor", "password": "professor"} + ) + assert authorized is not None + assert authorized["name"] == "professor" + async def test_ldap_auth_state_attributes(authenticator): authenticator.auth_state_attributes = ["employeeType"] From b5139f02edfd4c0fb70c84e7210cd8aba52220ef Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 17 Sep 2024 13:38:38 +0200 Subject: [PATCH 09/11] update docs for auth_state.user_attributes --- README.md | 2 +- ldapauthenticator/ldapauthenticator.py | 13 +++++++++---- ldapauthenticator/tests/test_ldapauthenticator.py | 4 ++-- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 172aeb7..ca96f12 100644 --- a/README.md +++ b/README.md @@ -244,7 +244,7 @@ are not escaped. #### `LDAPAuthenticator.auth_state_attributes` An optional list of attributes to be fetched for a user after login. -If found these will be returned as `auth_state`. +If found, these will be available as `auth_state["user_attributes"]`. #### `LDAPAuthenticator.use_lookup_dn_username` diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 34af803..95795cf 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -306,7 +306,12 @@ def _validate_bind_dn_template(self, proposal): attributes = List(config=True, help="List of attributes to be searched") auth_state_attributes = List( - config=True, help="List of attributes to be returned in auth_state for a user" + config=True, + help=""" + List of user attributes to be returned in auth_state + + Will be available in `auth_state["user_attributes"]` + """ ) use_lookup_dn_username = Bool( @@ -542,12 +547,12 @@ async def authenticate(self, handler, data): if not self.use_lookup_dn_username: username = data["username"] - user_attrs = self.get_user_attributes(conn, userdn) + user_attributes = self.get_user_attributes(conn, userdn) auth_state = { "ldap_groups": ldap_groups, - "user_attrs": user_attrs, + "user_attributes": user_attributes, } - self.log.debug("username:%s attributes:%s", username, user_attrs) + self.log.debug("username:%s attributes:%s", username, user_attributes) return {"name": username, "auth_state": auth_state} async def check_allowed(self, username, auth_model): diff --git a/ldapauthenticator/tests/test_ldapauthenticator.py b/ldapauthenticator/tests/test_ldapauthenticator.py index b875be6..03a83a3 100644 --- a/ldapauthenticator/tests/test_ldapauthenticator.py +++ b/ldapauthenticator/tests/test_ldapauthenticator.py @@ -167,7 +167,7 @@ async def test_ldap_auth_state_attributes(authenticator): None, {"username": "fry", "password": "fry"} ) assert authorized["name"] == "fry" - assert authorized["auth_state"]["user_attrs"] == {"employeeType": ["Delivery boy"]} + assert authorized["auth_state"]["user_attributes"] == {"employeeType": ["Delivery boy"]} async def test_ldap_auth_state_attributes2(authenticator): @@ -179,4 +179,4 @@ async def test_ldap_auth_state_attributes2(authenticator): None, {"username": "leela", "password": "leela"} ) assert authorized["name"] == "leela" - assert authorized["auth_state"]["user_attrs"] == {"description": ["Mutant"]} + assert authorized["auth_state"]["user_attributes"] == {"description": ["Mutant"]} From bb8474ba4ef3800ff24544645578e776502c876f Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Tue, 17 Sep 2024 11:40:49 +0000 Subject: [PATCH 10/11] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- ldapauthenticator/ldapauthenticator.py | 2 +- ldapauthenticator/tests/test_ldapauthenticator.py | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index 95795cf..e010180 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -311,7 +311,7 @@ def _validate_bind_dn_template(self, proposal): List of user attributes to be returned in auth_state Will be available in `auth_state["user_attributes"]` - """ + """, ) use_lookup_dn_username = Bool( diff --git a/ldapauthenticator/tests/test_ldapauthenticator.py b/ldapauthenticator/tests/test_ldapauthenticator.py index 03a83a3..9c037f3 100644 --- a/ldapauthenticator/tests/test_ldapauthenticator.py +++ b/ldapauthenticator/tests/test_ldapauthenticator.py @@ -125,6 +125,7 @@ async def test_ldap_auth_search_filter(authenticator): ) assert authorized is None + async def test_allow_config(authenticator): # test various sources of allow config @@ -167,7 +168,9 @@ async def test_ldap_auth_state_attributes(authenticator): None, {"username": "fry", "password": "fry"} ) assert authorized["name"] == "fry" - assert authorized["auth_state"]["user_attributes"] == {"employeeType": ["Delivery boy"]} + assert authorized["auth_state"]["user_attributes"] == { + "employeeType": ["Delivery boy"] + } async def test_ldap_auth_state_attributes2(authenticator): From 3f59d17a68341b1e1452bf4e238c0ea5802ee00e Mon Sep 17 00:00:00 2001 From: Min RK Date: Tue, 17 Sep 2024 13:51:45 +0200 Subject: [PATCH 11/11] handle allow_all variation for JupyterHub 4/5 --- ldapauthenticator/ldapauthenticator.py | 18 +++++++++++++----- .../tests/test_ldapauthenticator.py | 7 ++++++- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/ldapauthenticator/ldapauthenticator.py b/ldapauthenticator/ldapauthenticator.py index e010180..7326228 100644 --- a/ldapauthenticator/ldapauthenticator.py +++ b/ldapauthenticator/ldapauthenticator.py @@ -556,11 +556,19 @@ async def authenticate(self, handler, data): return {"name": username, "auth_state": auth_state} async def check_allowed(self, username, auth_model): - allowed = super().check_allowed(username, auth_model) - if isawaitable(allowed): - allowed = await allowed - if allowed is True: - return True + if not hasattr(self, "allow_all"): + # super for JupyterHub < 5 + # default behavior: no allow config => allow all + if not self.allowed_users and not self.allowed_groups: + return True + if self.allowed_users and username in self.allowed_users: + return True + else: + allowed = super().check_allowed(username, auth_model) + if isawaitable(allowed): + allowed = await allowed + if allowed is True: + return True if self.allowed_groups: # check allowed groups in_groups = set((auth_model.get("auth_state") or {}).get("ldap_groups", [])) diff --git a/ldapauthenticator/tests/test_ldapauthenticator.py b/ldapauthenticator/tests/test_ldapauthenticator.py index 9c037f3..254b259 100644 --- a/ldapauthenticator/tests/test_ldapauthenticator.py +++ b/ldapauthenticator/tests/test_ldapauthenticator.py @@ -153,7 +153,12 @@ async def test_allow_config(authenticator): ) assert authorized is None # allow_all grants access - authenticator.allow_all = True + if hasattr(authenticator, "allow_all"): + authenticator.allow_all = True + else: + # clear allow config for JupyterHub < 5 + authenticator.allowed_groups = [] + authenticator.allowed_users = set() authorized = await authenticator.get_authenticated_user( None, {"username": "professor", "password": "professor"} )