Kaynağa Gözat

Survive a directory that defines no posixGroup class (#2769)

Every LDAP user on an lldap directory was rejected with "Incorrect
username or password", on an install where Test Connection passed and
where the same bind DN, filter and group membership all checked out
under ldapsearch. The directory never saw the request.

_extract_user_info searches for POSIX groups alongside the memberOf
ones, and both of those filters name the posixGroup object class. ldap3
fetches the schema at connect time (get_info=ALL) and validates class
names in a filter against it while building the request, raising
LDAPObjectClassError before anything is sent. lldap marks every account
it creates as posixAccount -- which is what makes us look for POSIX
groups at all -- but defines no group class beyond groupOfNames. The
exception escaped authenticate_ldap_user, and the login route reports
any LDAP failure as bad credentials.

A directory with no posixGroup class has no posixGroup entries, which is
exactly the answer those searches would have returned. Catch it, log it
once, and carry on with the memberOf groups collected above. Both
searches sit inside the one try: they name the same class, so once one
is rejected the other cannot succeed, and attempting it would only
produce a second identical exception to swallow.

Not a regression from 848f55810. The memberUid filter has named the
class since b6599dd41 and runs for every user whether or not they have a
gidNumber, so a directory of this shape has never been able to log in;
the primary-group lookup only added a second trigger. Test Connection
was unaffected throughout because (objectClass=*) is a presence filter
and never reaches the value validator.

The mock connection gained a hook that raises on a filter substring,
standing in for that client-side validation. Reset in the fixture and
default None, so existing tests are unchanged.
maziggy 1 ay önce
ebeveyn
işleme
5efbd353ed

Dosya farkı çok büyük olduğundan ihmal edildi
+ 0 - 0
CHANGELOG.md


+ 42 - 18
backend/app/services/ldap_service.py

@@ -14,6 +14,7 @@ import logging
 from dataclasses import dataclass
 
 from ldap3 import ALL, SUBTREE, Connection, Server, Tls
+from ldap3.core.exceptions import LDAPObjectClassError
 
 logger = logging.getLogger(__name__)
 
@@ -155,32 +156,55 @@ def _extract_user_info(
 
     canonical_username = _pick_canonical_username(user_entry, fallback_username)
 
-    # Also search for POSIX groups (memberUid-based) using the service account
-    posix_filter = f"(&(objectClass=posixGroup)(memberUid={_ldap_escape(canonical_username)}))"
-    service_conn.search(
-        search_base=config.search_base,
-        search_filter=posix_filter,
-        search_scope=SUBTREE,
-        attributes=["cn"],
-    )
-    for entry in service_conn.entries:
-        groups.append(str(entry.entry_dn))
-
-    # POSIX primary group: user's gidNumber matches a posixGroup's gidNumber.
-    # Standard Unix semantics treat this as full group membership, so we need
-    # to resolve it to a group DN alongside the memberUid results.
-    if hasattr(user_entry, "gidNumber") and user_entry.gidNumber:
-        primary_gid = str(user_entry.gidNumber)
-        primary_filter = f"(&(objectClass=posixGroup)(gidNumber={_ldap_escape(primary_gid)}))"
+    # Also search for POSIX groups, both the memberUid kind and the primary
+    # gidNumber kind. Both filters name the posixGroup object class, and ldap3
+    # validates that name against the schema it fetched at connect time
+    # (get_info=ALL) before it builds the request — so on a directory that
+    # publishes a schema without posixGroup it raises client-side and nothing is
+    # ever sent. A directory with no posixGroup class has no posixGroup entries,
+    # which is exactly the answer the searches would have returned, so the
+    # correct response is to carry on with the memberOf groups collected above.
+    #
+    # Left uncaught, that exception escaped authenticate_ldap_user, and the login
+    # route reports any LDAP error as "Incorrect username or password" — so an
+    # lldap user, whose accounts carry posixAccount but whose directory defines
+    # no group classes beyond groupOfNames, could never log in and had nothing
+    # but a wrong-password message to go on (#2769). This predates the primary
+    # gidNumber lookup: the memberUid filter has named the class since #794.
+    try:
+        posix_filter = f"(&(objectClass=posixGroup)(memberUid={_ldap_escape(canonical_username)}))"
         service_conn.search(
             search_base=config.search_base,
-            search_filter=primary_filter,
+            search_filter=posix_filter,
             search_scope=SUBTREE,
             attributes=["cn"],
         )
         for entry in service_conn.entries:
             groups.append(str(entry.entry_dn))
 
+        # POSIX primary group: user's gidNumber matches a posixGroup's gidNumber.
+        # Standard Unix semantics treat this as full group membership, so we need
+        # to resolve it to a group DN alongside the memberUid results.
+        if hasattr(user_entry, "gidNumber") and user_entry.gidNumber:
+            primary_gid = str(user_entry.gidNumber)
+            primary_filter = f"(&(objectClass=posixGroup)(gidNumber={_ldap_escape(primary_gid)}))"
+            service_conn.search(
+                search_base=config.search_base,
+                search_filter=primary_filter,
+                search_scope=SUBTREE,
+                attributes=["cn"],
+            )
+            for entry in service_conn.entries:
+                groups.append(str(entry.entry_dn))
+    except LDAPObjectClassError:
+        # Logged once per authentication, at info: it is the explanation for a
+        # user's POSIX groups being absent from their mapping, and it is not an
+        # error the operator can or should act on.
+        logger.info(
+            "Directory publishes no posixGroup object class; skipping POSIX group lookup "
+            "(memberOf groups are unaffected)"
+        )
+
     # Dedupe group DNs (user may be in a group via both memberUid and primary gidNumber).
     # Case-insensitive comparison — LDAP DNs are case-insensitive by spec.
     seen_lower: set[str] = set()

+ 88 - 0
backend/tests/unit/services/test_ldap_service.py

@@ -11,6 +11,7 @@ are not tested here — they require a live LDAP server.
 """
 
 import pytest
+from ldap3.core.exceptions import LDAPObjectClassError
 
 from backend.app.services.ldap_service import (
     LDAPConfig,
@@ -297,6 +298,11 @@ class _MockConnection:
 
     _search_fixture: dict[str, list] = {}
     _instances: list["_MockConnection"] = []
+    # Filter substring that should raise LDAPObjectClassError instead of
+    # searching, standing in for ldap3's client-side schema validation — it
+    # rejects an object class the server's published schema doesn't define
+    # before the request is ever built (#2769).
+    _raise_object_class_error_on: str | None = None
 
     def __init__(self, *args, **kwargs):
         self.entries: list = []
@@ -320,6 +326,9 @@ class _MockConnection:
         # **kwargs absorbs ldap3 options like size_limit that the real client supports
         self.search_calls.append(search_filter or "")
         self.last_attrs = list(attributes) if attributes is not None else None
+        needle = _MockConnection._raise_object_class_error_on
+        if needle and needle in (search_filter or ""):
+            raise LDAPObjectClassError(f"invalid class in objectClass attribute: {needle}")
         for needle, entries in _MockConnection._search_fixture.items():
             if needle in (search_filter or ""):
                 self.entries = entries
@@ -333,6 +342,7 @@ def mock_ldap(monkeypatch):
     """Patch Connection + _create_server in ldap_service so authenticate_ldap_user can run offline."""
     _MockConnection._search_fixture = {}
     _MockConnection._instances = []
+    _MockConnection._raise_object_class_error_on = None
     monkeypatch.setattr("backend.app.services.ldap_service.Connection", _MockConnection)
     monkeypatch.setattr("backend.app.services.ldap_service._create_server", lambda config: None)
     return _MockConnection
@@ -433,6 +443,84 @@ class TestAuthenticateLdapUserGroups:
         assert gidnumber_searches == []
 
 
+class TestDirectoryWithoutPosixGroupClass:
+    """A directory whose published schema defines no posixGroup class (#2769).
+
+    ldap3 fetches the schema at connect time (get_info=ALL) and validates object
+    class names in a filter against it before building the request, so both POSIX
+    group searches raise client-side and nothing reaches the server. lldap is the
+    case in the wild: it puts posixAccount on every account it creates, which
+    gives each user a gidNumber, but defines no group class beyond groupOfNames.
+    Left uncaught the exception escaped authenticate_ldap_user and the login route
+    reported it as "Incorrect username or password", so LDAP login was impossible.
+    """
+
+    def test_authenticates_and_keeps_memberof_groups(self, mock_ldap):
+        """The reporter's setup: the mapped membership comes from memberOf, which
+        is read off the user entry and never touches a posixGroup filter."""
+        user_entry = _MockEntry(
+            "uid=peter,ou=people,dc=fablab,dc=test",
+            uid="peter",
+            gidNumber=1001,  # lldap gives every account one
+            memberOf=["cn=AAUStudents,ou=groups,dc=fablab,dc=test"],
+        )
+        mock_ldap._search_fixture = {"(uid=peter)": [user_entry]}
+        mock_ldap._raise_object_class_error_on = "objectClass=posixGroup"
+
+        info = authenticate_ldap_user(_base_config(), "peter", "password")
+
+        assert info is not None
+        assert info.groups == ["cn=AAUStudents,ou=groups,dc=fablab,dc=test"]
+
+    def test_authenticates_with_no_groups_at_all(self, mock_ldap):
+        """No memberOf either. The user still gets in — auto-provisioning assigns
+        the configured default group, which is the whole point of that setting."""
+        user_entry = _MockEntry("uid=peter,ou=people,dc=fablab,dc=test", uid="peter", gidNumber=1001)
+        mock_ldap._search_fixture = {"(uid=peter)": [user_entry]}
+        mock_ldap._raise_object_class_error_on = "objectClass=posixGroup"
+
+        info = authenticate_ldap_user(_base_config(), "peter", "password")
+
+        assert info is not None
+        assert info.username == "peter"
+        assert info.groups == []
+
+    def test_abandons_the_primary_gid_search_after_the_first_rejection(self, mock_ldap):
+        """Both filters name the same class, so once one is rejected the other
+        cannot succeed. Attempting it would only produce a second identical
+        exception to swallow."""
+        user_entry = _MockEntry("uid=peter,ou=people,dc=fablab,dc=test", uid="peter", gidNumber=1001)
+        mock_ldap._search_fixture = {"(uid=peter)": [user_entry]}
+        mock_ldap._raise_object_class_error_on = "objectClass=posixGroup"
+
+        authenticate_ldap_user(_base_config(), "peter", "password")
+
+        service_conn = _MockConnection._instances[0]
+        posix_searches = [call for call in service_conn.search_calls if "posixGroup" in call]
+        assert len(posix_searches) == 1
+        assert "memberUid=peter" in posix_searches[0]
+
+    def test_a_directory_that_defines_the_class_is_untouched(self, mock_ldap):
+        """The guard must not cost a normal directory its POSIX groups — both
+        searches still run and both results still land."""
+        user_entry = _MockEntry("cn=mz,dc=test,dc=com", uid="mz", gidNumber=10002)
+        supplementary = _MockEntry("cn=bambuddy-viewers,ou=groups,dc=test,dc=com")
+        primary = _MockEntry("cn=bambuddy-operators,ou=groups,dc=test,dc=com")
+
+        mock_ldap._search_fixture = {
+            "(uid=mz)": [user_entry],
+            "memberUid=mz": [supplementary],
+            "gidNumber=10002": [primary],
+        }
+
+        info = authenticate_ldap_user(_base_config(), "mz", "password")
+
+        assert info.groups == [
+            "cn=bambuddy-viewers,ou=groups,dc=test,dc=com",
+            "cn=bambuddy-operators,ou=groups,dc=test,dc=com",
+        ]
+
+
 # ---------------------------------------------------------------------------
 # Manual provisioning helpers — search_ldap_users + lookup_ldap_user (#1298)
 # ---------------------------------------------------------------------------

Bu fark içinde çok fazla dosya değişikliği olduğu için bazı dosyalar gösterilmiyor