diff --git a/backend/app/services/ldap_service.py b/backend/app/services/ldap_service.py index 8f45fef84..710338c36 100644 --- a/backend/app/services/ldap_service.py +++ b/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() diff --git a/backend/tests/unit/services/test_ldap_service.py b/backend/tests/unit/services/test_ldap_service.py index 33ad167a8..f6b85ee8e 100644 --- a/backend/tests/unit/services/test_ldap_service.py +++ b/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) # ---------------------------------------------------------------------------