From 8edbbe7b3cd98b0b8ebecd65aebf1c4c4e475a4d Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Tue, 5 Nov 2019 02:24:18 +0100 Subject: [PATCH] ldap: Make email search config obligatory without LDAP_APPEND_DOMAIN. Having to account everywhere for both cases of having and not having email search configured makes things needlessly complicated. It's better to make the setting obligatory in configurations other than LDAP_APPEND_DOMAIN. --- zerver/tests/test_auth_backends.py | 42 ------------------------------ zproject/backends.py | 16 +++++++----- 2 files changed, 9 insertions(+), 49 deletions(-) diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 9b44281c26..b152345c0a 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -2416,14 +2416,6 @@ class DjangoToLDAPUsernameTests(ZulipTestCase): username = self.backend.django_to_ldap_username('"hamlet@test"@zulip') self.assertEqual(username, '"hamlet@test"@zulip') - def test_django_to_ldap_username_without_email_search(self) -> None: - with self.settings(AUTH_LDAP_REVERSE_EMAIL_SEARCH=None): - self.assertEqual(self.backend.django_to_ldap_username("hamlet"), "hamlet") - self.assertEqual(self.backend.django_to_ldap_username("newuser_email_as_uid@zulip.com"), - "newuser_email_as_uid@zulip.com") - with self.assertRaises(ZulipLDAPExceptionNoMatchingLDAPUser): - self.backend.django_to_ldap_username("hamlet@example.com") - def test_django_to_ldap_username_with_email_search(self) -> None: self.assertEqual(self.backend.django_to_ldap_username("hamlet"), self.ldap_username("hamlet")) @@ -2481,25 +2473,7 @@ class DjangoToLDAPUsernameTests(ZulipTestCase): user_profile.set_password(password) user_profile.save() - # Without email search, can't login via ldap: - with self.settings(AUTH_LDAP_REVERSE_EMAIL_SEARCH=None, LDAP_EMAIL_ATTR='mail'): - # Using hamlet's ldap password fails without email search: - self.assertEqual( - authenticate(username=user_profile.email, password=self.ldap_password(), realm=realm), - None) - # Need hamlet's zulip password to login (via email backend) - self.assertEqual( - authenticate(username=user_profile.email, password="testpassword", realm=realm), - user_profile) - # To login via ldap, username needs to be the ldap username, not email: - self.assertEqual( - authenticate(username=self.ldap_username("hamlet"), password=self.ldap_password(), - realm=realm), - user_profile) - - # With email search: with self.settings(LDAP_EMAIL_ATTR='mail'): - # Ldap password works now: self.assertEqual( authenticate(username=user_profile.email, password=self.ldap_password(), realm=realm), user_profile) @@ -2523,11 +2497,6 @@ class DjangoToLDAPUsernameTests(ZulipTestCase): # 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) - class ZulipLDAPTestCase(ZulipTestCase): def setUp(self) -> None: super().setUp() @@ -2642,17 +2611,6 @@ class TestLDAP(ZulipLDAPTestCase): realm=realm) self.assertEqual(user_profile, othello) - @override_settings(AUTH_LDAP_REVERSE_EMAIL_SEARCH=None, LDAP_APPEND_DOMAIN=None, - AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',)) - def test_no_append_domain_and_email_search_not_configured(self) -> None: - with mock.patch("zproject.backends.logging.warning") as mock_warning: - result = email_belongs_to_ldap(get_realm("zulip"), "nonexistant@email.com") - self.assertEqual(result, True) - mock_warning.assert_called_with( - "LDAP_APPEND_DOMAIN isn't used, but searching by email " - "is not configured. email_belongs_to_ldap will always return True." - ) - @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',)) def test_login_failure_due_to_wrong_password(self) -> None: with self.settings(LDAP_APPEND_DOMAIN='zulip.com'): diff --git a/zproject/backends.py b/zproject/backends.py index 56875c9208..2c4feb9e42 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -246,6 +246,11 @@ def is_valid_email(email: str) -> bool: return False return True +def check_ldap_config() -> None: + if not settings.LDAP_APPEND_DOMAIN: + # Email search needs to be configured in this case. + assert settings.AUTH_LDAP_USERNAME_ATTR and settings.AUTH_LDAP_REVERSE_EMAIL_SEARCH + def find_ldap_users_by_email(email: str) -> Optional[List[_LDAPUser]]: """ Returns list of _LDAPUsers matching the email search, @@ -265,16 +270,12 @@ def email_belongs_to_ldap(realm: Realm, email: str) -> bool: if not ldap_auth_enabled(realm): return False + check_ldap_config() if settings.LDAP_APPEND_DOMAIN: # Check if the email ends with LDAP_APPEND_DOMAIN return email.strip().lower().endswith("@" + settings.LDAP_APPEND_DOMAIN) # If we don't have an LDAP domain, we have to do a lookup for the email. - if not(settings.AUTH_LDAP_USERNAME_ATTR and settings.AUTH_LDAP_REVERSE_EMAIL_SEARCH): - logging.warning("LDAP_APPEND_DOMAIN isn't used, but searching by email " - "is not configured. email_belongs_to_ldap will always return True.") - return True - if find_ldap_users_by_email(email): return True else: @@ -313,6 +314,8 @@ class ZulipLDAPAuthBackendBase(ZulipAuthMixin, LDAPBackend): if settings.DEVELOPMENT and settings.FAKE_LDAP_MODE: # nocoverage init_fakeldap() + check_ldap_config() + # Disable django-auth-ldap's permissions functions -- we don't use # the standard Django user/group permissions system because they # are prone to performance issues. @@ -342,8 +345,7 @@ class ZulipLDAPAuthBackendBase(ZulipAuthMixin, LDAPBackend): raise ZulipLDAPExceptionOutsideDomain("Email %s does not match LDAP domain %s." % ( username, settings.LDAP_APPEND_DOMAIN)) result = email_to_username(username) - - elif settings.AUTH_LDAP_USERNAME_ATTR and settings.AUTH_LDAP_REVERSE_EMAIL_SEARCH: + else: # We can use find_ldap_users_by_email if is_valid_email(username): email_search_result = find_ldap_users_by_email(username)