Skip to content

Commit

Permalink
auth: Remove short_name from LDAP API.
Browse files Browse the repository at this point in the history
As best I can tell, we fetched this field and then ignored it, so
unlike the last few commits, this is more a code cleanup than a
functional change.
  • Loading branch information
timabbott committed Jul 17, 2020
1 parent c445001 commit 6be3fca
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 38 deletions.
26 changes: 11 additions & 15 deletions zerver/tests/test_auth_backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -3403,7 +3403,7 @@ def totp(*args: Any, **kwargs: Any) -> int:

# Setup LDAP
self.init_default_ldap_database()
ldap_user_attr_map = {'full_name': 'cn', 'short_name': 'sn'}
ldap_user_attr_map = {'full_name': 'cn'}
with self.settings(
AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',),
TWO_FACTOR_CALL_GATEWAY='two_factor.gateways.fake.Fake',
Expand Down Expand Up @@ -4204,9 +4204,9 @@ class _LDAPUser:
@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
def test_get_or_build_user_when_user_does_not_exist(self) -> None:
class _LDAPUser:
attrs = {'fn': ['Full Name'], 'sn': ['Short Name']}
attrs = {'fn': ['Full Name']}

ldap_user_attr_map = {'full_name': 'fn', 'short_name': 'sn'}
ldap_user_attr_map = {'full_name': 'fn'}

with self.settings(AUTH_LDAP_USER_ATTR_MAP=ldap_user_attr_map):
backend = self.backend
Expand All @@ -4219,9 +4219,9 @@ class _LDAPUser:
@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
def test_get_or_build_user_when_user_has_invalid_name(self) -> None:
class _LDAPUser:
attrs = {'fn': ['<invalid name>'], 'sn': ['Short Name']}
attrs = {'fn': ['<invalid name>']}

ldap_user_attr_map = {'full_name': 'fn', 'short_name': 'sn'}
ldap_user_attr_map = {'full_name': 'fn'}

with self.settings(AUTH_LDAP_USER_ATTR_MAP=ldap_user_attr_map):
backend = self.backend
Expand All @@ -4232,9 +4232,9 @@ class _LDAPUser:
@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
def test_get_or_build_user_when_realm_is_deactivated(self) -> None:
class _LDAPUser:
attrs = {'fn': ['Full Name'], 'sn': ['Short Name']}
attrs = {'fn': ['Full Name']}

ldap_user_attr_map = {'full_name': 'fn', 'short_name': 'sn'}
ldap_user_attr_map = {'full_name': 'fn'}

with self.settings(AUTH_LDAP_USER_ATTR_MAP=ldap_user_attr_map):
backend = self.backend
Expand Down Expand Up @@ -4305,7 +4305,7 @@ def test_login_failure_when_domain_does_not_match(self) -> None:

@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
def test_login_success_with_different_subdomain(self) -> None:
ldap_user_attr_map = {'full_name': 'cn', 'short_name': 'sn'}
ldap_user_attr_map = {'full_name': 'cn'}

Realm.objects.create(string_id='acme')
with self.settings(
Expand Down Expand Up @@ -4695,30 +4695,26 @@ def test_user_not_present(self) -> None:
@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
def test_normal_query(self) -> None:
with self.settings(AUTH_LDAP_USER_ATTR_MAP={'full_name': 'cn',
'short_name': 'sn',
'avatar': 'jpegPhoto',
'custom_profile_field__birthday': 'birthDate',
'custom_profile_field__phone_number': 'nonExistentAttr',
}):
values = query_ldap(self.example_email('hamlet'))
self.assertEqual(len(values), 5)
self.assertEqual(len(values), 4)
self.assertIn('full_name: King Hamlet', values)
self.assertIn('short_name: Hamlet', values)
self.assertIn('avatar: (An avatar image file)', values)
self.assertIn('custom_profile_field__birthday: 1900-09-08', values)
self.assertIn('custom_profile_field__phone_number: LDAP field not present', values)

@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
def test_query_email_attr(self) -> None:
with self.settings(AUTH_LDAP_USER_ATTR_MAP={'full_name': 'cn',
'short_name': 'sn'},
with self.settings(AUTH_LDAP_USER_ATTR_MAP={'full_name': 'cn'},
LDAP_EMAIL_ATTR='mail'):
# This will look up the user by email in our test dictionary,
# should successfully find hamlet's ldap entry.
values = query_ldap(self.example_email('hamlet'))
self.assertEqual(len(values), 3)
self.assertEqual(len(values), 2)
self.assertIn('full_name: King Hamlet', values)
self.assertIn('short_name: Hamlet', values)
self.assertIn('email: [email protected]', values)

class TestZulipAuthMixin(ZulipTestCase):
Expand Down
6 changes: 3 additions & 3 deletions zerver/tests/test_email_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ def test_day1_email_context(self) -> None:
"(uid=%(email)s)"))
def test_day1_email_ldap_case_a_login_credentials(self) -> None:
self.init_default_ldap_database()
ldap_user_attr_map = {'full_name': 'cn', 'short_name': 'sn'}
ldap_user_attr_map = {'full_name': 'cn'}

with self.settings(AUTH_LDAP_USER_ATTR_MAP=ldap_user_attr_map):
self.login_with_return("[email protected]",
Expand All @@ -155,7 +155,7 @@ def test_day1_email_ldap_case_a_login_credentials(self) -> None:
'zproject.backends.ZulipDummyBackend'))
def test_day1_email_ldap_case_b_login_credentials(self) -> None:
self.init_default_ldap_database()
ldap_user_attr_map = {'full_name': 'cn', 'short_name': 'sn'}
ldap_user_attr_map = {'full_name': 'cn'}

with self.settings(
LDAP_APPEND_DOMAIN='zulip.com',
Expand All @@ -175,7 +175,7 @@ def test_day1_email_ldap_case_b_login_credentials(self) -> None:
'zproject.backends.ZulipDummyBackend'))
def test_day1_email_ldap_case_c_login_credentials(self) -> None:
self.init_default_ldap_database()
ldap_user_attr_map = {'full_name': 'cn', 'short_name': 'sn'}
ldap_user_attr_map = {'full_name': 'cn'}

with self.settings(
LDAP_EMAIL_ATTR='mail',
Expand Down
10 changes: 4 additions & 6 deletions zerver/tests/test_signup.py
Original file line number Diff line number Diff line change
Expand Up @@ -3340,7 +3340,7 @@ def test_ldap_registration_end_to_end(self) -> None:
subdomain = "zulip"

self.init_default_ldap_database()
ldap_user_attr_map = {'full_name': 'cn', 'short_name': 'sn'}
ldap_user_attr_map = {'full_name': 'cn'}
full_name = 'New LDAP fullname'

with patch('zerver.views.registration.get_subdomain', return_value=subdomain):
Expand Down Expand Up @@ -3452,7 +3452,6 @@ def test_ldap_auto_registration_on_login(self) -> None:
self.init_default_ldap_database()
ldap_user_attr_map = {
'full_name': 'cn',
'short_name': 'sn',
'custom_profile_field__phone_number': 'homePhone',
}
full_name = 'New LDAP fullname'
Expand Down Expand Up @@ -3483,7 +3482,6 @@ def test_ldap_registration_multiple_realms(self) -> None:
self.init_default_ldap_database()
ldap_user_attr_map = {
'full_name': 'cn',
'short_name': 'sn',
}
do_create_realm('test', 'test', False)

Expand Down Expand Up @@ -3517,7 +3515,7 @@ def test_ldap_registration_when_names_changes_are_disabled(self) -> None:
subdomain = "zulip"

self.init_default_ldap_database()
ldap_user_attr_map = {'full_name': 'cn', 'short_name': 'sn'}
ldap_user_attr_map = {'full_name': 'cn'}

with patch('zerver.views.registration.get_subdomain', return_value=subdomain):
result = self.client_post('/register/', {'email': email})
Expand Down Expand Up @@ -3561,7 +3559,7 @@ def test_signup_with_ldap_and_email_enabled_using_email_with_ldap_append_domain(
subdomain = "zulip"

self.init_default_ldap_database()
ldap_user_attr_map = {'full_name': 'cn', 'short_name': 'sn'}
ldap_user_attr_map = {'full_name': 'cn'}

with patch('zerver.views.registration.get_subdomain', return_value=subdomain):
result = self.client_post('/register/', {'email': email})
Expand Down Expand Up @@ -3660,7 +3658,7 @@ def test_signup_with_ldap_and_email_enabled_using_email_with_ldap_email_search(s
subdomain = "zulip"

self.init_default_ldap_database()
ldap_user_attr_map = {'full_name': 'cn', 'short_name': 'sn'}
ldap_user_attr_map = {'full_name': 'cn'}

with patch('zerver.views.registration.get_subdomain', return_value=subdomain):
result = self.client_post('/register/', {'email': email})
Expand Down
2 changes: 1 addition & 1 deletion zerver/views/registration.py
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ def accounts_register(request: HttpRequest) -> HttpResponse:
ldap_user = _LDAPUser(backend, ldap_username)

try:
ldap_full_name, _ = backend.get_mapped_name(ldap_user)
ldap_full_name = backend.get_mapped_name(ldap_user)
except TypeError:
break

Expand Down
22 changes: 9 additions & 13 deletions zproject/backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -562,30 +562,26 @@ def is_account_control_disabled_user(self, ldap_user: _LDAPUser) -> bool:
return ldap_disabled

@classmethod
def get_mapped_name(cls, ldap_user: _LDAPUser) -> Tuple[str, str]:
"""Constructs the user's Zulip full_name and short_name fields from
the LDAP data"""
def get_mapped_name(cls, ldap_user: _LDAPUser) -> str:
"""Constructs the user's Zulip full_name from the LDAP data"""
if "full_name" in settings.AUTH_LDAP_USER_ATTR_MAP:
full_name_attr = settings.AUTH_LDAP_USER_ATTR_MAP["full_name"]
short_name = full_name = ldap_user.attrs[full_name_attr][0]
full_name = ldap_user.attrs[full_name_attr][0]
elif all(key in settings.AUTH_LDAP_USER_ATTR_MAP for key in {"first_name", "last_name"}):
first_name_attr = settings.AUTH_LDAP_USER_ATTR_MAP["first_name"]
last_name_attr = settings.AUTH_LDAP_USER_ATTR_MAP["last_name"]
short_name = ldap_user.attrs[first_name_attr][0]
full_name = short_name + ' ' + ldap_user.attrs[last_name_attr][0]
first_name = ldap_user.attrs[first_name_attr][0]
last_name = ldap_user.attrs[last_name_attr][0]
full_name = f"{first_name} {last_name}"
else:
raise ZulipLDAPException("Missing required mapping for user's full name")

if "short_name" in settings.AUTH_LDAP_USER_ATTR_MAP:
short_name_attr = settings.AUTH_LDAP_USER_ATTR_MAP["short_name"]
short_name = ldap_user.attrs[short_name_attr][0]

return full_name, short_name
return full_name

def sync_full_name_from_ldap(self, user_profile: UserProfile,
ldap_user: _LDAPUser) -> None:
from zerver.lib.actions import do_change_full_name
full_name, _ = self.get_mapped_name(ldap_user)
full_name = self.get_mapped_name(ldap_user)
if full_name != user_profile.full_name:
try:
full_name = check_full_name(full_name)
Expand Down Expand Up @@ -731,7 +727,7 @@ def get_or_build_user(self, username: str, ldap_user: _LDAPUser) -> Tuple[UserPr
raise ZulipLDAPException("Email validation failed.")

# We have valid LDAP credentials; time to create an account.
full_name, short_name = self.get_mapped_name(ldap_user)
full_name = self.get_mapped_name(ldap_user)
try:
full_name = check_full_name(full_name)
except JsonableError as e:
Expand Down

0 comments on commit 6be3fca

Please sign in to comment.