Merge branch 'main' into fix-escape_userdn

.github/workflows/release.yaml

@@ -23,13 +23,13 @@ on:
 
 jobs:
   build-release:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-latest
 
     steps:
-      - uses: actions/checkout@v3
-      - uses: actions/setup-python@v4
+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v5
         with:
-          python-version: "3.11"
+          python-version: "3.12"
 
       - name: install build package
         run: |

.github/workflows/test.yaml

@@ -25,28 +25,29 @@ env:
 
 jobs:
   test:
-    runs-on: ubuntu-22.04
+    runs-on: ubuntu-latest
     timeout-minutes: 10
 
     strategy:
       fail-fast: false
       matrix:
-        python-version:
-          - "3.7"
-          - "3.8"
-          - "3.9"
-          - "3.10"
-          - "3.11"
+        include:
+          - python-version: "3.9"
+            pip-install-spec: "jupyterhub==4.*"
+          - python-version: "3.12"
+            pip-install-spec: "jupyterhub==5.*"
+          - python-version: "3.x"
+            pip-install-spec: "--pre jupyterhub"
 
     steps:
-      - uses: actions/checkout@v3
-      - uses: actions/setup-python@v4
+      - uses: actions/checkout@v4
+      - uses: actions/setup-python@v5
         with:
           python-version: "${{ matrix.python-version }}"
 
       - name: Install Python dependencies
         run: |
-          pip install --upgrade pip
+          pip install ${{ matrix.pip-install-spec }}
           pip install -e ".[test]"
 
       - name: List packages
@@ -60,4 +61,4 @@ jobs:
           pytest --cov=ldapauthenticator
 
       # GitHub action reference: https://github.com/codecov/codecov-action
-      - uses: codecov/codecov-action@v3
+      - uses: codecov/codecov-action@v4

.pre-commit-config.yaml

@@ -11,11 +11,11 @@
 repos:
   # Autoformat: Python code, syntax patterns are modernized
   - repo: https://github.com/asottile/pyupgrade
-    rev: v3.15.2
+    rev: v3.17.0
     hooks:
       - id: pyupgrade
         args:
-          - --py38-plus
+          - --py39-plus
 
   # Autoformat: Python code
   - repo: https://github.com/PyCQA/autoflake
@@ -34,7 +34,7 @@ repos:
 
   # Autoformat: Python code
   - repo: https://github.com/psf/black
-    rev: 24.4.2
+    rev: 24.8.0
     hooks:
       - id: black
 
@@ -55,7 +55,7 @@ repos:
 
   # Lint: Python code
   - repo: https://github.com/pycqa/flake8
-    rev: "7.0.0"
+    rev: "7.1.1"
     hooks:
       - id: flake8
 

CONTRIBUTING.md

@@ -9,8 +9,6 @@ This guide was adapted from the [contributing guide in the main `jupyterhub` rep
 
 ## Setting up a development environment
 
-JupyterHub requires Python >= 3.7.
-
 As a Python project, a development install of JupyterHub follows standard practices for installation and testing.
 
 Note: if you have Docker installed locally, you can run all of the subsequent commands inside of a container after you run the following initial commands:

README.md

@@ -36,11 +36,14 @@ that this will log out _all_ users who are currently logged in.
 
 ## Usage
 
-You can enable this authenticator with the following lines in your
-`jupyter_config.py`:
+You can enable this authenticator by adding lines to your `jupyterhub_config.py`.
+
+**Note: This file may not exist in your current installation! In TLJH, it
+is located in /opt/tljh/config/jupyterhub_config.d. Create it there if you
+don't already have one.**
 
 ```python
-c.JupyterHub.authenticator_class = 'ldapauthenticator.LDAPAuthenticator'
+c.JupyterHub.authenticator_class = 'ldap'
 ```
 
 ### Required configuration

ci/docker-ldap.sh

@@ -5,7 +5,7 @@ set -e
 # container based on the image rroemhild/test-openldap.
 #
 # ref: https://github.com/rroemhild/docker-test-openldap
-# ref: https://hub.docker.com/r/rroemhild/test-openldap/
+# ref: https://github.com/rroemhild/docker-test-openldap/pkgs/container/docker-test-openldap
 #
 # Stop any existing test-openldap container
 docker rm --force test-openldap 2>/dev/null || true
@@ -15,4 +15,11 @@ docker rm --force test-openldap 2>/dev/null || true
 # - 389:10389 (ldap)
 # - 636:10636 (ldaps)
 #
-docker run --detach --name=test-openldap -p 389:10389 -p 636:10636 rroemhild/test-openldap:2.1
+# Image updated 2024-09-12 to the latest commit's build
+# https://github.com/rroemhild/docker-test-openldap/commit/2645f2164ffb51ec4b5b4a9af0065ad7f2ffc1cf
+#
+IMAGE=ghcr.io/rroemhild/docker-test-openldap@sha256:107ecba713dd233f6f84047701d1b4dda03307d972814f2ae1db69b0d250544f
+docker run --detach --name=test-openldap -p 389:10389 -p 636:10636 $IMAGE
+
+# It takes a bit more than one second for the container to become ready
+sleep 3

ldapauthenticator/__init__.py

@@ -3,4 +3,4 @@
 # __version__ should be updated using tbump, based on configuration in
 # pyproject.toml, according to instructions in RELEASE.md.
 #
-__version__ = "1.3.3.dev"
+__version__ = "2.0.0.dev"

ldapauthenticator/ldapauthenticator.py

@@ -4,7 +4,7 @@
 from jupyterhub.auth import Authenticator
 from ldap3.utils.conv import escape_filter_chars
 from tornado import gen
-from traitlets import Bool, Int, List, Unicode, Union
+from traitlets import Bool, Int, List, Unicode, Union, validate
 
 
 class LDAPAuthenticator(Authenticator):
@@ -65,6 +65,19 @@ def _server_port_default(self):
         """,
     )
 
+    @validate("bind_dn_template")
+    def _validate_bind_dn_template(self, proposal):
+        """
+        Ensure a List[str] is set, filtered from empty string elements.
+        """
+        rv = []
+        if isinstance(proposal.value, str):
+            rv = [proposal.value]
+        if "" in rv:
+            self.log.warning("Ignoring blank 'bind_dn_template' entry!")
+            rv = [e for e in rv if e]
+        return rv
+
     allowed_groups = List(
         config=True,
         allow_none=True,
@@ -131,7 +144,7 @@ def _server_port_default(self):
         c.LDAPAuthenticator.lookup_dn_search_user = 'ldap_search_user_technical_account'
         c.LDAPAuthenticator.lookup_dn_search_password = 'secret'
         c.LDAPAuthenticator.user_search_base = 'ou=people,dc=wikimedia,dc=org'
-        c.LDAPAuthenticator.user_attribute = 'sAMAccountName'
+        c.LDAPAuthenticator.user_attribute = 'uid'
         c.LDAPAuthenticator.lookup_dn_user_dn_attribute = 'cn'
         c.LDAPAuthenticator.bind_dn_template = '{username}'
         ```
@@ -226,36 +239,32 @@ def _server_port_default(self):
     )
 
     def resolve_username(self, username_supplied_by_user):
+        """
+        Resolves a username supplied by a user to the a user DN when lookup_dn
+        is True.
+        """
         search_dn = self.lookup_dn_search_user
         if self.escape_userdn:
             search_dn = escape_filter_chars(search_dn)
         conn = self.get_connection(
-            userdn=search_dn, password=self.lookup_dn_search_password
+            userdn=search_dn,
+            password=self.lookup_dn_search_password,
         )
-        is_bound = conn.bind()
-        if not is_bound:
-            msg = "Failed to connect to LDAP server with search user '{search_dn}'"
-            self.log.warning(msg.format(search_dn=search_dn))
+        if not conn.bind():
+            self.log.warning(
+                f"Failed to connect to LDAP server with search user '{search_dn}'"
+            )
             return (None, None)
 
         search_filter = self.lookup_dn_search_filter.format(
             login_attr=self.user_attribute,
             login=escape_filter_chars(username_supplied_by_user),
         )
-        msg = "\n".join(
-            [
-                "Looking up user with:",
-                "    search_base = '{search_base}'",
-                "    search_filter = '{search_filter}'",
-                "    attributes = '{attributes}'",
-            ]
-        )
         self.log.debug(
-            msg.format(
-                search_base=self.user_search_base,
-                search_filter=search_filter,
-                attributes=self.user_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,
@@ -265,14 +274,9 @@ def resolve_username(self, username_supplied_by_user):
         )
         response = conn.response
         if len(response) == 0 or "attributes" not in response[0].keys():
-            msg = (
-                "No entry found for user '{username}' "
-                "when looking up attribute '{attribute}'"
-            )
             self.log.warning(
-                msg.format(
-                    username=username_supplied_by_user, attribute=self.user_attribute
-                )
+                f"No entry found for user '{username_supplied_by_user}' "
+                f"when looking up attribute '{self.user_attribute}'"
             )
             return (None, None)
 
@@ -283,19 +287,11 @@ def resolve_username(self, username_supplied_by_user):
             elif len(user_dn) == 1:
                 user_dn = user_dn[0]
             else:
-                msg = (
-                    "A lookup of the username '{username}' returned a list "
-                    "of entries for the attribute '{attribute}'. Only the "
-                    "first among these ('{first_entry}') was used. The other "
-                    "entries ({other_entries}) were ignored."
-                )
                 self.log.warn(
-                    msg.format(
-                        username=username_supplied_by_user,
-                        attribute=self.lookup_dn_user_dn_attribute,
-                        first_entry=user_dn[0],
-                        other_entries=", ".join(user_dn[1:]),
-                    )
+                    f"A lookup of the username '{username_supplied_by_user}' returned a list "
+                    f"of entries for the attribute '{self.lookup_dn_user_dn_attribute}'. Only "
+                    f"the first among these ('{user_dn[0]}') was used. The other entries "
+                    f"({', '.join(user_dn[1:])}) were ignored."
                 )
                 user_dn = user_dn[0]
 
@@ -325,6 +321,14 @@ def get_user_attributes(self, conn, userdn):
 
     @gen.coroutine
     def authenticate(self, handler, data):
+        """
+        Note: This function is really meant to identify a user, and
+              check_allowed and check_blocked are meant to determine if its an
+              authorized user. Authorization is currently handled by returning
+              None here instead.
+
+        ref: https://jupyterhub.readthedocs.io/en/latest/reference/authenticators.html#authenticator-authenticate
+        """
         username = data["username"]
         password = data["password"]
 
@@ -342,18 +346,14 @@ def authenticate(self, handler, data):
             self.log.warning("username:%s Login denied for blank password", username)
             return None
 
-        # bind_dn_template should be of type List[str]
-        bind_dn_template = self.bind_dn_template
-        if isinstance(bind_dn_template, str):
-            bind_dn_template = [bind_dn_template]
-
         # sanity check
-        if not self.lookup_dn and not bind_dn_template:
+        if not self.lookup_dn and not self.bind_dn_template:
             self.log.warning(
                 "Login not allowed, please configure 'lookup_dn' or 'bind_dn_template'."
             )
             return None
 
+        bind_dn_template = self.bind_dn_template
         if self.lookup_dn:
             username, resolved_dn = self.resolve_username(username)
             if not username:
@@ -366,14 +366,10 @@ def authenticate(self, handler, data):
 
         is_bound = False
         for dn in bind_dn_template:
-            if not dn:
-                self.log.warning("Ignoring blank 'bind_dn_template' entry!")
-                continue
             userdn = dn.format(username=username)
             if self.escape_userdn:
                 userdn = escape_filter_chars(userdn)
-            msg = "Attempting to bind {username} with {userdn}"
-            self.log.debug(msg.format(username=username, userdn=userdn))
+            self.log.debug(f"Attempting to bind {username} with {userdn}")
             msg = "Status of user bind {username} with {userdn} : {is_bound}"
             try:
                 conn = self.get_connection(userdn, password)
@@ -391,8 +387,7 @@ def authenticate(self, handler, data):
                 break
 
         if not is_bound:
-            msg = "Invalid password for user '{username}'"
-            self.log.warning(msg.format(username=username))
+            self.log.warning(f"Invalid password for user '{username}'")
             return None
 
         if self.search_filter:
@@ -407,20 +402,14 @@ def authenticate(self, handler, data):
             )
             n_users = len(conn.response)
             if n_users == 0:
-                msg = "User with '{userattr}={username}' not found in directory"
                 self.log.warning(
-                    msg.format(userattr=self.user_attribute, username=username)
+                    f"User with '{self.user_attribute}={username}' not found in directory"
                 )
                 return None
             if n_users > 1:
-                msg = (
-                    "Duplicate users found! "
-                    "{n_users} users found with '{userattr}={username}'"
-                )
                 self.log.warning(
-                    msg.format(
-                        userattr=self.user_attribute, username=username, n_users=n_users
-                    )
+                    "Duplicate users found! {n_users} users found "
+                    f"with '{self.user_attribute}={username}'"
                 )
                 return None
 
@@ -450,8 +439,9 @@ def authenticate(self, handler, data):
                     break
             if not found:
                 # If we reach here, then none of the groups matched
-                msg = "username:{username} User not in any of the allowed groups"
-                self.log.warning(msg.format(username=username))
+                self.log.warning(
+                    f"username:{username} User not in any of the allowed groups"
+                )
                 return None
 
         if not self.use_lookup_dn_username: