Merge pull request #269 from minrk/allowed_groups

align allowed_groups with other `allowed_` config, consistent in JupyterHub 5

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`
 

ldapauthenticator/ldapauthenticator.py

@@ -1,5 +1,6 @@
 import enum
 import re
+from inspect import isawaitable
 
 import ldap3
 from jupyterhub.auth import Authenticator
@@ -142,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.
         """,
     )
 
@@ -286,13 +290,28 @@ 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")
 
     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(
@@ -328,10 +347,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,
@@ -499,14 +518,14 @@ 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(
                     "Missing group_search_filter or group_attributes. Both are required."
                 )
                 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,19 +537,49 @@ async def authenticate(self, handler, data):
                     attributes=self.group_attributes,
                 )
                 if found:
+                    ldap_groups.append(group)
+                    # we currently only use this in check_allowed,
+                    # 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 found:
-                # 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"]
 
-        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_attributes = self.get_user_attributes(conn, userdn)
+        auth_state = {
+            "ldap_groups": ldap_groups,
+            "user_attributes": user_attributes,
+        }
+        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):
+        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", []))
+            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
+        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

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
@@ -124,14 +126,56 @@ 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
+    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"}
+    )
+    assert authorized is not None
+    assert authorized["name"] == "professor"
+
+
 async def test_ldap_auth_state_attributes(authenticator):
     authenticator.auth_state_attributes = ["employeeType"]
     # proper username and password in allowed group
     authorized = await authenticator.get_authenticated_user(
         None, {"username": "fry", "password": "fry"}
     )
     assert authorized["name"] == "fry"
-    assert authorized["auth_state"] == {"employeeType": ["Delivery boy"]}
+    assert authorized["auth_state"]["user_attributes"] == {
+        "employeeType": ["Delivery boy"]
+    }
 
 
 async def test_ldap_auth_state_attributes2(authenticator):
@@ -143,4 +187,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_attributes"] == {"description": ["Mutant"]}