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

authenticate: Use keyword-only parameters. 

Since positional arguments are interpreted differently by different
backends in Django's authentication backend system, it’s safer to
disallow them.

This had been the motivation for previously declaring the parameters
with default values when we were on Python 2, but that was not super
effective because Python has no rule against positional default
arguments and that convention for our authentication backends was
solely enforced by code review.

Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This commit is contained in:
Anders Kaseorg 2019-05-07 17:18:34 -07:00 committed by Tim Abbott
parent 082f23a659
commit 802d3dbbf4
2 changed files with 26 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

38
zerver/tests/test_auth_backends.py
View File

@ -178,7 +178,7 @@ class AuthBackendTest(ZulipTestCase):
mock.patch('zproject.backends.password_auth_enabled', mock.patch('zproject.backends.password_auth_enabled',
return_value=True): return_value=True):
return_data = {} # type: Dict[str, bool] return_data = {} # type: Dict[str, bool]
user = EmailAuthBackend().authenticate(self.example_email('hamlet'), user = EmailAuthBackend().authenticate(username=self.example_email('hamlet'),
realm=get_realm("zulip"), realm=get_realm("zulip"),
password=password, password=password,
return_data=return_data) return_data=return_data)
@ -211,8 +211,8 @@ class AuthBackendTest(ZulipTestCase):
user_profile.save() user_profile.save()
# Verify if a realm has password auth disabled, correct password is rejected # Verify if a realm has password auth disabled, correct password is rejected
with mock.patch('zproject.backends.password_auth_enabled', return_value=False): with mock.patch('zproject.backends.password_auth_enabled', return_value=False):
self.assertIsNone(EmailAuthBackend().authenticate(self.example_email('hamlet'), self.assertIsNone(EmailAuthBackend().authenticate(username=self.example_email('hamlet'),
password, password=password,
realm=get_realm("zulip"))) realm=get_realm("zulip")))
def test_login_preview(self) -> None: def test_login_preview(self) -> None:
@ -314,7 +314,7 @@ class AuthBackendTest(ZulipTestCase):
mock.patch('django_auth_ldap.backend._LDAPUser._check_requirements')), ( mock.patch('django_auth_ldap.backend._LDAPUser._check_requirements')), (
mock.patch('django_auth_ldap.backend._LDAPUser.attrs', mock.patch('django_auth_ldap.backend._LDAPUser.attrs',
return_value=dict(full_name=['Hamlet']))): return_value=dict(full_name=['Hamlet']))):
self.assertIsNone(backend.authenticate(email, password, realm=get_realm("zulip"))) self.assertIsNone(backend.authenticate(username=email, password=password, realm=get_realm("zulip")))
with mock.patch('django_auth_ldap.backend._LDAPUser._authenticate_user_dn'), ( with mock.patch('django_auth_ldap.backend._LDAPUser._authenticate_user_dn'), (
mock.patch('django_auth_ldap.backend._LDAPUser._check_requirements')), ( mock.patch('django_auth_ldap.backend._LDAPUser._check_requirements')), (
@ -412,11 +412,11 @@ class AuthBackendTest(ZulipTestCase):
} }
} # type: Dict[str, Any] } # type: Dict[str, Any]
def patched_authenticate(*args: Any, **kwargs: Any) -> Any: def patched_authenticate(**kwargs: Any) -> Any:
if 'subdomain' in kwargs: if 'subdomain' in kwargs:
backend.strategy.session_set("subdomain", kwargs["subdomain"]) backend.strategy.session_set("subdomain", kwargs["subdomain"])
del kwargs['subdomain'] del kwargs['subdomain']
result = orig_authenticate(backend, *args, **kwargs) result = orig_authenticate(backend, **kwargs)
return result return result
for backend_name in backends_to_test: for backend_name in backends_to_test:
@ -2406,7 +2406,7 @@ class TestLDAP(ZulipLDAPTestCase):
LDAP_APPEND_DOMAIN='zulip.com', LDAP_APPEND_DOMAIN='zulip.com',
AUTH_LDAP_BIND_PASSWORD='', AUTH_LDAP_BIND_PASSWORD='',
AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'): AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'):
user_profile = self.backend.authenticate('ldapuser1', 'dapu', user_profile = self.backend.authenticate(username='ldapuser1', password='dapu',
realm=get_realm('zulip')) realm=get_realm('zulip'))
assert(user_profile is None) assert(user_profile is None)
@ -2422,7 +2422,7 @@ class TestLDAP(ZulipLDAPTestCase):
LDAP_APPEND_DOMAIN='zulip.com', LDAP_APPEND_DOMAIN='zulip.com',
AUTH_LDAP_BIND_PASSWORD='', AUTH_LDAP_BIND_PASSWORD='',
AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'): AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'):
user_profile = self.backend.authenticate(self.example_email("hamlet"), 'testing', user_profile = self.backend.authenticate(username=self.example_email("hamlet"), password='testing',
realm=get_realm('zulip')) realm=get_realm('zulip'))
assert(user_profile is not None) assert(user_profile is not None)
@ -2439,7 +2439,7 @@ class TestLDAP(ZulipLDAPTestCase):
LDAP_APPEND_DOMAIN='zulip.com', LDAP_APPEND_DOMAIN='zulip.com',
AUTH_LDAP_BIND_PASSWORD='', AUTH_LDAP_BIND_PASSWORD='',
AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'): AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'):
user_profile = self.backend.authenticate("hamlet", 'testing', user_profile = self.backend.authenticate(username="hamlet", password='testing',
realm=get_realm('zulip')) realm=get_realm('zulip'))
assert(user_profile is not None) assert(user_profile is not None)
@ -2456,7 +2456,7 @@ class TestLDAP(ZulipLDAPTestCase):
with self.settings(LDAP_EMAIL_ATTR='email', with self.settings(LDAP_EMAIL_ATTR='email',
AUTH_LDAP_BIND_PASSWORD='', AUTH_LDAP_BIND_PASSWORD='',
AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'): AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'):
user_profile = self.backend.authenticate("letham", 'testing', user_profile = self.backend.authenticate(username="letham", password='testing',
realm=get_realm('zulip')) realm=get_realm('zulip'))
assert (user_profile is not None) assert (user_profile is not None)
@ -2473,7 +2473,7 @@ class TestLDAP(ZulipLDAPTestCase):
LDAP_APPEND_DOMAIN='zulip.com', LDAP_APPEND_DOMAIN='zulip.com',
AUTH_LDAP_BIND_PASSWORD='', AUTH_LDAP_BIND_PASSWORD='',
AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'): AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'):
user = self.backend.authenticate(self.example_email("hamlet"), 'wrong', user = self.backend.authenticate(username=self.example_email("hamlet"), password='wrong',
realm=get_realm('zulip')) realm=get_realm('zulip'))
self.assertIs(user, None) self.assertIs(user, None)
@ -2488,7 +2488,7 @@ class TestLDAP(ZulipLDAPTestCase):
LDAP_APPEND_DOMAIN='zulip.com', LDAP_APPEND_DOMAIN='zulip.com',
AUTH_LDAP_BIND_PASSWORD='', AUTH_LDAP_BIND_PASSWORD='',
AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'): AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'):
user = self.backend.authenticate('nonexistent@zulip.com', 'testing', user = self.backend.authenticate(username='nonexistent@zulip.com', password='testing',
realm=get_realm('zulip')) realm=get_realm('zulip'))
self.assertIs(user, None) self.assertIs(user, None)
@ -2645,7 +2645,7 @@ class TestLDAP(ZulipLDAPTestCase):
@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',)) @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
def test_login_failure_when_domain_does_not_match(self) -> None: def test_login_failure_when_domain_does_not_match(self) -> None:
with self.settings(LDAP_APPEND_DOMAIN='acme.com'): with self.settings(LDAP_APPEND_DOMAIN='acme.com'):
user_profile = self.backend.authenticate(self.example_email("hamlet"), 'pass', user_profile = self.backend.authenticate(username=self.example_email("hamlet"), password='pass',
realm=get_realm('zulip')) realm=get_realm('zulip'))
self.assertIs(user_profile, None) self.assertIs(user_profile, None)
@ -2666,7 +2666,7 @@ class TestLDAP(ZulipLDAPTestCase):
AUTH_LDAP_BIND_PASSWORD='', AUTH_LDAP_BIND_PASSWORD='',
AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com', AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com',
AUTH_LDAP_USER_ATTR_MAP=ldap_user_attr_map): AUTH_LDAP_USER_ATTR_MAP=ldap_user_attr_map):
user_profile = self.backend.authenticate(self.example_email('hamlet'), 'testing', user_profile = self.backend.authenticate(username=self.example_email('hamlet'), password='testing',
realm=get_realm('acme')) realm=get_realm('acme'))
self.assertEqual(user_profile.email, self.example_email('hamlet')) self.assertEqual(user_profile.email, self.example_email('hamlet'))
@ -2681,7 +2681,7 @@ class TestLDAP(ZulipLDAPTestCase):
LDAP_APPEND_DOMAIN='zulip.com', LDAP_APPEND_DOMAIN='zulip.com',
AUTH_LDAP_BIND_PASSWORD='', AUTH_LDAP_BIND_PASSWORD='',
AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'): AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'):
user_profile = self.backend.authenticate(self.example_email("hamlet"), 'testing', user_profile = self.backend.authenticate(username=self.example_email("hamlet"), password='testing',
realm=get_realm('zulip')) realm=get_realm('zulip'))
assert(user_profile is not None) assert(user_profile is not None)
self.assertEqual(user_profile.email, self.example_email("hamlet")) self.assertEqual(user_profile.email, self.example_email("hamlet"))
@ -2699,7 +2699,7 @@ class TestLDAP(ZulipLDAPTestCase):
LDAP_APPEND_DOMAIN='zulip.com', LDAP_APPEND_DOMAIN='zulip.com',
AUTH_LDAP_BIND_PASSWORD='', AUTH_LDAP_BIND_PASSWORD='',
AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'): AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'):
user_profile = self.backend.authenticate(self.example_email("hamlet"), 'testing', user_profile = self.backend.authenticate(username=self.example_email("hamlet"), password='testing',
realm=get_realm('zulip')) realm=get_realm('zulip'))
self.assertIs(user_profile, None) self.assertIs(user_profile, None)
@ -2721,7 +2721,7 @@ class TestLDAP(ZulipLDAPTestCase):
LDAP_APPEND_DOMAIN='acme.com', LDAP_APPEND_DOMAIN='acme.com',
AUTH_LDAP_BIND_PASSWORD='', AUTH_LDAP_BIND_PASSWORD='',
AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=acme,dc=com'): AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=acme,dc=com'):
user_profile = self.backend.authenticate('nonexisting@acme.com', 'testing', user_profile = self.backend.authenticate(username='nonexisting@acme.com', password='testing',
realm=get_realm('zulip')) realm=get_realm('zulip'))
assert(user_profile is not None) assert(user_profile is not None)
self.assertEqual(user_profile.email, 'nonexisting@acme.com') self.assertEqual(user_profile.email, 'nonexisting@acme.com')
@ -2747,7 +2747,7 @@ class TestLDAP(ZulipLDAPTestCase):
AUTH_LDAP_BIND_PASSWORD='', AUTH_LDAP_BIND_PASSWORD='',
AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com', AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com',
AUTH_LDAP_USER_ATTR_MAP={'first_name': 'fn', 'last_name': 'ln'}): AUTH_LDAP_USER_ATTR_MAP={'first_name': 'fn', 'last_name': 'ln'}):
user_profile = self.backend.authenticate('nonexisting@zulip.com', 'testing', user_profile = self.backend.authenticate(username='nonexisting@zulip.com', password='testing',
realm=get_realm('zulip')) realm=get_realm('zulip'))
assert(user_profile is not None) assert(user_profile is not None)
self.assertEqual(user_profile.email, 'nonexisting@zulip.com') self.assertEqual(user_profile.email, 'nonexisting@zulip.com')
@ -2757,7 +2757,7 @@ class TestLDAP(ZulipLDAPTestCase):
class TestZulipLDAPUserPopulator(ZulipLDAPTestCase): class TestZulipLDAPUserPopulator(ZulipLDAPTestCase):
def test_authenticate(self) -> None: def test_authenticate(self) -> None:
backend = ZulipLDAPUserPopulator() backend = ZulipLDAPUserPopulator()
result = backend.authenticate(self.example_email("hamlet"), 'testing', result = backend.authenticate(username=self.example_email("hamlet"), password='testing',
realm=get_realm('zulip')) realm=get_realm('zulip'))
self.assertIs(result, None) self.assertIs(result, None)

14
zproject/backends.py
View File

@ -154,7 +154,7 @@ class ZulipDummyBackend(ZulipAuthMixin):
when explicitly requested by including the use_dummy_backend kwarg. when explicitly requested by including the use_dummy_backend kwarg.
""" """
def authenticate(self, username: str, realm: Realm, def authenticate(self, *, username: str, realm: Realm,
use_dummy_backend: bool=False, use_dummy_backend: bool=False,
return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]: return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]:
if use_dummy_backend: if use_dummy_backend:
@ -168,7 +168,7 @@ class EmailAuthBackend(ZulipAuthMixin):
Allows a user to sign in using an email/password pair. Allows a user to sign in using an email/password pair.
""" """
def authenticate(self, username: str, password: str, def authenticate(self, *, username: str, password: str,
realm: Realm, realm: Realm,
return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]: return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]:
""" Authenticate a user based on email address as the user name. """ """ Authenticate a user based on email address as the user name. """
@ -201,7 +201,7 @@ class GoogleMobileOauth2Backend(ZulipAuthMixin):
https://developers.google.com/accounts/docs/CrossClientAuth#offlineAccess https://developers.google.com/accounts/docs/CrossClientAuth#offlineAccess
""" """
def authenticate(self, google_oauth2_token: str, realm: Realm, def authenticate(self, *, google_oauth2_token: str, realm: Realm,
return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]: return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]:
# We lazily import apiclient as part of optimizing the base # We lazily import apiclient as part of optimizing the base
# import time for a Zulip management command, since it's only # import time for a Zulip management command, since it's only
@ -238,7 +238,7 @@ class ZulipRemoteUserBackend(RemoteUserBackend):
""" """
create_unknown_user = False create_unknown_user = False
def authenticate(self, remote_user: str, realm: Realm, def authenticate(self, *, remote_user: str, realm: Realm,
return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]: return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]:
if not auth_enabled_helper(["RemoteUser"], realm): if not auth_enabled_helper(["RemoteUser"], realm):
return None return None
@ -456,7 +456,7 @@ class ZulipLDAPAuthBackendBase(ZulipAuthMixin, LDAPBackend):
class ZulipLDAPAuthBackend(ZulipLDAPAuthBackendBase): class ZulipLDAPAuthBackend(ZulipLDAPAuthBackendBase):
REALM_IS_NONE_ERROR = 1 REALM_IS_NONE_ERROR = 1
def authenticate(self, username: str, password: str, realm: Realm, def authenticate(self, *, username: str, password: str, realm: Realm,
prereg_user: Optional[PreregistrationUser]=None, prereg_user: Optional[PreregistrationUser]=None,
return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]: return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]:
self._realm = realm self._realm = realm
@ -574,7 +574,7 @@ class ZulipLDAPUserPopulator(ZulipLDAPAuthBackendBase):
registration for organizations that use a different SSO solution registration for organizations that use a different SSO solution
for managing login (often via RemoteUserBackend). for managing login (often via RemoteUserBackend).
""" """
def authenticate(self, username: str, password: str, realm: Realm, def authenticate(self, *, username: str, password: str, realm: Realm,
return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]: return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]:
return None return None
@ -611,7 +611,7 @@ def query_ldap(email: str) -> List[str]:
class DevAuthBackend(ZulipAuthMixin): class DevAuthBackend(ZulipAuthMixin):
"""Allow logging in as any user without a password. This is used for """Allow logging in as any user without a password. This is used for
convenience when developing Zulip, and is disabled in production.""" convenience when developing Zulip, and is disabled in production."""
def authenticate(self, dev_auth_username: str, realm: Realm, def authenticate(self, *, dev_auth_username: str, realm: Realm,
return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]: return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]:
if not dev_auth_enabled(realm): if not dev_auth_enabled(realm):
return None return None