diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 2319835023..0f672de9a2 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -3,7 +3,7 @@ from django.conf import settings from django.core import mail from django.http import HttpResponse from django.test import override_settings -from django_auth_ldap.backend import LDAPBackend, LDAPSearch +from django_auth_ldap.backend import LDAPBackend, LDAPSearch, _LDAPUser from django.test.client import RequestFactory from django.utils.timezone import now as timezone_now from typing import Any, Callable, Dict, List, Optional, Set, Tuple @@ -2546,10 +2546,11 @@ class TestLDAP(ZulipLDAPTestCase): backend.django_to_ldap_username('"hamlet@test"@test.com') @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',)) - def test_ldap_to_django_username(self) -> None: + def test_user_email_from_ldapuser_with_append_domain(self) -> None: backend = self.backend with self.settings(LDAP_APPEND_DOMAIN='zulip.com'): - username = backend.ldap_to_django_username('"hamlet@test"') + username = backend.user_email_from_ldapuser('this_argument_is_ignored', + _LDAPUser(self.backend, username='"hamlet@test"')) self.assertEqual(username, '"hamlet@test"@zulip.com') @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',)) diff --git a/zerver/tests/test_ldap.py b/zerver/tests/test_ldap.py index c3c77e1713..3fbb206ed7 100644 --- a/zerver/tests/test_ldap.py +++ b/zerver/tests/test_ldap.py @@ -4,7 +4,7 @@ from django.test.utils import override_settings from zerver.lib.test_classes import ZulipTestCase from zerver.models import get_realm -from zproject.backends import ZulipLDAPAuthBackend, ZulipLDAPExceptionOutsideDomain +from zproject.backends import ZulipLDAPAuthBackend, ZulipLDAPExceptionOutsideDomain, sync_user_from_ldap from django_auth_ldap.config import LDAPSearch @@ -89,3 +89,27 @@ class DjangoToLDAPUsernameTests(ZulipTestCase): # Ldap password works now: self.assertEqual(authenticate(username=user_profile.email, password="testing", realm=realm), user_profile) + + @override_settings(LDAP_EMAIL_ATTR='mail', LDAP_DEACTIVATE_NON_MATCHING_USERS=True) + def test_sync_user_from_ldap_with_email_attr(self) -> None: + """In LDAP configurations with LDAP_EMAIL_ATTR configured and + LDAP_DEACTIVATE_NON_MATCHING_USERS set, a possible failure + mode if django_to_ldap_username isn't configured correctly is + all LDAP users having their accounts deactivated. Before the + introduction of AUTH_LDAP_REVERSE_EMAIL_SEARCH, this would happen + even in valid LDAP configurations using LDAP_EMAIL_ATTR. + + This test confirms that such a failure mode doesn't happen with + a valid LDAP configuration. + """ + + user_profile = self.example_user("hamlet") + with self.settings(): + sync_user_from_ldap(user_profile, mock.Mock()) + # Syncing didn't deactivate the user: + self.assertTrue(user_profile.is_active) + + with self.settings(AUTH_LDAP_REVERSE_EMAIL_SEARCH=None): + # Without email search, the user will get deactivated. + sync_user_from_ldap(user_profile, mock.Mock()) + self.assertFalse(user_profile.is_active) diff --git a/zproject/backends.py b/zproject/backends.py index 93c3153f16..68dd55d9d6 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -348,9 +348,35 @@ class ZulipLDAPAuthBackendBase(ZulipAuthMixin, LDAPBackend): return username - def ldap_to_django_username(self, username: str) -> str: + def user_email_from_ldapuser(self, username: str, ldap_user: _LDAPUser) -> str: + if hasattr(ldap_user, '_username'): + # In tests, we sometimes pass a simplified _LDAPUser without _username attr, + # and with the intended username in the username argument. + username = ldap_user._username + if settings.LDAP_APPEND_DOMAIN: return "@".join((username, settings.LDAP_APPEND_DOMAIN)) + + if settings.LDAP_EMAIL_ATTR is not None: + # Get email from ldap attributes. + if settings.LDAP_EMAIL_ATTR not in ldap_user.attrs: + raise ZulipLDAPException("LDAP user doesn't have the needed %s attribute" % ( + settings.LDAP_EMAIL_ATTR,)) + else: + return ldap_user.attrs[settings.LDAP_EMAIL_ATTR][0] + + return username + + def ldap_to_django_username(self, username: str) -> str: + """ + This is called inside django_auth_ldap with only one role: + to convert _LDAPUser._username to django username (so in Zulip, the email) + and pass that as "username" argument to get_or_build_user(username, ldapuser). + In many cases, the email is stored in the _LDAPUser's attributes, so it can't be + constructed just from the username. We choose to do nothing in this function, + and our overrides of get_or_build_user() obtain that username from the _LDAPUser + object on their own, through our user_email_from_ldapuser function. + """ return username def sync_avatar_from_ldap(self, user: UserProfile, ldap_user: _LDAPUser) -> None: @@ -478,7 +504,13 @@ class ZulipLDAPAuthBackendBase(ZulipAuthMixin, LDAPBackend): ./manage.py sync_ldap_user_data In authentication contexts, this is overriden in ZulipLDAPAuthBackend. """ + # Obtain the django username from the ldap_user object: + username = self.user_email_from_ldapuser(username, ldap_user) + + # Call the library get_or_build_user for building the UserProfile + # with the username we obtained: (user, built) = super().get_or_build_user(username, ldap_user) + # Synchronise the UserProfile with its LDAP attributes: if 'userAccountControl' in settings.AUTH_LDAP_USER_ATTR_MAP: user_disabled_in_ldap = self.is_account_control_disabled_user(ldap_user) if user_disabled_in_ldap: @@ -549,14 +581,7 @@ class ZulipLDAPAuthBackend(ZulipLDAPAuthBackendBase): """ return_data = {} # type: Dict[str, Any] - if settings.LDAP_EMAIL_ATTR is not None: - # Get email from ldap attributes. - if settings.LDAP_EMAIL_ATTR not in ldap_user.attrs: - return_data["ldap_missing_attribute"] = settings.LDAP_EMAIL_ATTR - raise ZulipLDAPException("LDAP user doesn't have the needed %s attribute" % ( - settings.LDAP_EMAIL_ATTR,)) - - username = ldap_user.attrs[settings.LDAP_EMAIL_ATTR][0] + username = self.user_email_from_ldapuser(username, ldap_user) if 'userAccountControl' in settings.AUTH_LDAP_USER_ATTR_MAP: # nocoverage ldap_disabled = self.is_account_control_disabled_user(ldap_user)