Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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.
This commit is contained in:
Mateusz Mandera 2019-11-05 02:24:18 +01:00 committed by Tim Abbott
parent bfe800b11a
commit 8edbbe7b3c
2 changed files with 9 additions and 49 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

42
zerver/tests/test_auth_backends.py
View File

@ -2416,14 +2416,6 @@ class DjangoToLDAPUsernameTests(ZulipTestCase):
username = self.backend.django_to_ldap_username('"hamlet@test"@zulip') username = self.backend.django_to_ldap_username('"hamlet@test"@zulip')
self.assertEqual(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: def test_django_to_ldap_username_with_email_search(self) -> None:
self.assertEqual(self.backend.django_to_ldap_username("hamlet"), self.assertEqual(self.backend.django_to_ldap_username("hamlet"),
self.ldap_username("hamlet")) self.ldap_username("hamlet"))
@ -2481,25 +2473,7 @@ class DjangoToLDAPUsernameTests(ZulipTestCase):
user_profile.set_password(password) user_profile.set_password(password)
user_profile.save() 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'): with self.settings(LDAP_EMAIL_ATTR='mail'):
# Ldap password works now:
self.assertEqual( self.assertEqual(
authenticate(username=user_profile.email, password=self.ldap_password(), realm=realm), authenticate(username=user_profile.email, password=self.ldap_password(), realm=realm),
user_profile) user_profile)
@ -2523,11 +2497,6 @@ class DjangoToLDAPUsernameTests(ZulipTestCase):
# Syncing didn't deactivate the user: # Syncing didn't deactivate the user:
self.assertTrue(user_profile.is_active) 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): class ZulipLDAPTestCase(ZulipTestCase):
def setUp(self) -> None: def setUp(self) -> None:
super().setUp() super().setUp()
@ -2642,17 +2611,6 @@ class TestLDAP(ZulipLDAPTestCase):
realm=realm) realm=realm)
self.assertEqual(user_profile, othello) 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',)) @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
def test_login_failure_due_to_wrong_password(self) -> None: def test_login_failure_due_to_wrong_password(self) -> None:
with self.settings(LDAP_APPEND_DOMAIN='zulip.com'): with self.settings(LDAP_APPEND_DOMAIN='zulip.com'):

16
zproject/backends.py
View File

@ -246,6 +246,11 @@ def is_valid_email(email: str) -> bool:
return False return False
return True 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]]: def find_ldap_users_by_email(email: str) -> Optional[List[_LDAPUser]]:
""" """
Returns list of _LDAPUsers matching the email search, 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): if not ldap_auth_enabled(realm):
return False return False
check_ldap_config()
if settings.LDAP_APPEND_DOMAIN: if settings.LDAP_APPEND_DOMAIN:
# Check if the email ends with LDAP_APPEND_DOMAIN # Check if the email ends with LDAP_APPEND_DOMAIN
return email.strip().lower().endswith("@" + settings.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 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): if find_ldap_users_by_email(email):
return True return True
else: else:
@ -313,6 +314,8 @@ class ZulipLDAPAuthBackendBase(ZulipAuthMixin, LDAPBackend):
if settings.DEVELOPMENT and settings.FAKE_LDAP_MODE: # nocoverage if settings.DEVELOPMENT and settings.FAKE_LDAP_MODE: # nocoverage
init_fakeldap() init_fakeldap()
check_ldap_config()
# Disable django-auth-ldap's permissions functions -- we don't use # Disable django-auth-ldap's permissions functions -- we don't use
# the standard Django user/group permissions system because they # the standard Django user/group permissions system because they
# are prone to performance issues. # are prone to performance issues.
@ -342,8 +345,7 @@ class ZulipLDAPAuthBackendBase(ZulipAuthMixin, LDAPBackend):
raise ZulipLDAPExceptionOutsideDomain("Email %s does not match LDAP domain %s." % ( raise ZulipLDAPExceptionOutsideDomain("Email %s does not match LDAP domain %s." % (
username, settings.LDAP_APPEND_DOMAIN)) username, settings.LDAP_APPEND_DOMAIN))
result = email_to_username(username) result = email_to_username(username)
else:
elif settings.AUTH_LDAP_USERNAME_ATTR and settings.AUTH_LDAP_REVERSE_EMAIL_SEARCH:
# We can use find_ldap_users_by_email # We can use find_ldap_users_by_email
if is_valid_email(username): if is_valid_email(username):
email_search_result = find_ldap_users_by_email(username) email_search_result = find_ldap_users_by_email(username)