paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-17 12:21:58 +00:00
Code Issues Packages Projects Releases Wiki Activity

authenticate: Remove default values for required parameters.

Browse Source 
It is now the caller’s responsibility to check that realm is not None.

Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This commit is contained in:
Anders Kaseorg
2019-05-04 16:04:48 -07:00
committed by Tim Abbott
parent 725582850f
commit 082f23a659
4 changed files with 40 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
zerver/forms.py
View File

@@ -280,9 +280,12 @@ class OurAuthenticationForm(AuthenticationForm):
if username is not None and password: if username is not None and password:
subdomain = get_subdomain(self.request) subdomain = get_subdomain(self.request)
try: try:
realm = get_realm(subdomain) # type: Optional[Realm] realm = get_realm(subdomain)
except Realm.DoesNotExist: except Realm.DoesNotExist:
realm = None logging.warning("User %s attempted to password login to nonexistent subdomain %s" %
(username, subdomain))
raise ValidationError("Realm does not exist")
return_data = {} # type: Dict[str, Any] return_data = {} # type: Dict[str, Any]
self.user_cache = authenticate(self.request, username=username, password=password, self.user_cache = authenticate(self.request, username=username, password=password,
realm=realm, return_data=return_data) realm=realm, return_data=return_data)

50
zerver/tests/test_auth_backends.py
View File

@@ -201,7 +201,7 @@ class AuthBackendTest(ZulipTestCase):
return_data=dict()), return_data=dict()),
bad_kwargs=dict(password=password, bad_kwargs=dict(password=password,
username=username, username=username,
realm=None, realm=get_realm('zephyr'),
return_data=dict())) return_data=dict()))
def test_email_auth_backend_disabled_password_auth(self) -> None: def test_email_auth_backend_disabled_password_auth(self) -> None:
@@ -268,15 +268,18 @@ class AuthBackendTest(ZulipTestCase):
with mock.patch('apiclient.sample_tools.client.verify_id_token', return_value=payload): with mock.patch('apiclient.sample_tools.client.verify_id_token', return_value=payload):
self.verify_backend(backend, self.verify_backend(backend,
good_kwargs=dict(realm=get_realm("zulip")), good_kwargs=dict(google_oauth2_token="",
bad_kwargs=dict(realm=None)) realm=get_realm("zulip")),
bad_kwargs=dict(google_oauth2_token="",
realm=get_realm("zephyr")))
# Verify valid_attestation parameter is set correctly # Verify valid_attestation parameter is set correctly
unverified_payload = dict(email_verified=False) unverified_payload = dict(email_verified=False)
with mock.patch('apiclient.sample_tools.client.verify_id_token', with mock.patch('apiclient.sample_tools.client.verify_id_token',
return_value=unverified_payload): return_value=unverified_payload):
ret = dict() # type: Dict[str, str] ret = dict() # type: Dict[str, str]
result = backend.authenticate(realm=get_realm("zulip"), return_data=ret) result = backend.authenticate(
google_oauth2_token="", realm=get_realm("zulip"), return_data=ret)
self.assertIsNone(result) self.assertIsNone(result)
self.assertFalse(ret["valid_attestation"]) self.assertFalse(ret["valid_attestation"])
@@ -284,13 +287,15 @@ class AuthBackendTest(ZulipTestCase):
with mock.patch('apiclient.sample_tools.client.verify_id_token', with mock.patch('apiclient.sample_tools.client.verify_id_token',
return_value=nonexistent_user_payload): return_value=nonexistent_user_payload):
ret = dict() ret = dict()
result = backend.authenticate(realm=get_realm("zulip"), return_data=ret) result = backend.authenticate(
google_oauth2_token="", realm=get_realm("zulip"), return_data=ret)
self.assertIsNone(result) self.assertIsNone(result)
self.assertTrue(ret["valid_attestation"]) self.assertTrue(ret["valid_attestation"])
with mock.patch('apiclient.sample_tools.client.verify_id_token', with mock.patch('apiclient.sample_tools.client.verify_id_token',
side_effect=AppIdentityError): side_effect=AppIdentityError):
ret = dict() ret = dict()
result = backend.authenticate(realm=get_realm("zulip"), return_data=ret) result = backend.authenticate(
google_oauth2_token="", realm=get_realm("zulip"), return_data=ret)
self.assertIsNone(result) self.assertIsNone(result)
@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',)) @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
@@ -325,7 +330,7 @@ class AuthBackendTest(ZulipTestCase):
self.verify_backend(backend, self.verify_backend(backend,
bad_kwargs=dict(username=username, bad_kwargs=dict(username=username,
password=password, password=password,
realm=None), realm=get_realm('zephyr')),
good_kwargs=dict(username=username, good_kwargs=dict(username=username,
password=password, password=password,
realm=get_realm('zulip'))) realm=get_realm('zulip')))
@@ -335,7 +340,7 @@ class AuthBackendTest(ZulipTestCase):
good_kwargs=dict(dev_auth_username=self.get_username(), good_kwargs=dict(dev_auth_username=self.get_username(),
realm=get_realm("zulip")), realm=get_realm("zulip")),
bad_kwargs=dict(dev_auth_username=self.get_username(), bad_kwargs=dict(dev_auth_username=self.get_username(),
realm=None)) realm=get_realm("zephyr")))
@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipRemoteUserBackend',)) @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipRemoteUserBackend',))
def test_remote_user_backend(self) -> None: def test_remote_user_backend(self) -> None:
@@ -353,7 +358,7 @@ class AuthBackendTest(ZulipTestCase):
good_kwargs=dict(remote_user=username, good_kwargs=dict(remote_user=username,
realm=get_realm('zulip')), realm=get_realm('zulip')),
bad_kwargs=dict(remote_user=username, bad_kwargs=dict(remote_user=username,
realm=None)) realm=get_realm('zephyr')))
@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipRemoteUserBackend',)) @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipRemoteUserBackend',))
@override_settings(SSO_APPEND_DOMAIN='zulip.com') @override_settings(SSO_APPEND_DOMAIN='zulip.com')
@@ -2468,7 +2473,8 @@ 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(self.example_email("hamlet"), 'wrong',
realm=get_realm('zulip'))
self.assertIs(user, None) self.assertIs(user, None)
@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',)) @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
@@ -2482,7 +2488,8 @@ 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('nonexistent@zulip.com', 'testing',
realm=get_realm('zulip'))
self.assertIs(user, None) self.assertIs(user, None)
@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',)) @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
@@ -2638,7 +2645,8 @@ 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(self.example_email("hamlet"), 'pass',
realm=get_realm('zulip'))
self.assertIs(user_profile, None) self.assertIs(user_profile, None)
@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',)) @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
@@ -2662,21 +2670,6 @@ class TestLDAP(ZulipLDAPTestCase):
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'))
@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
def test_login_failure_due_to_invalid_subdomain(self) -> None:
self.mock_ldap.directory = {
'uid=hamlet,ou=users,dc=zulip,dc=com': {
'userPassword': ['testing', ]
}
}
with self.settings(
LDAP_APPEND_DOMAIN='zulip.com',
AUTH_LDAP_BIND_PASSWORD='',
AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com'):
user_profile = self.backend.authenticate(self.example_email("hamlet"), 'testing',
realm=None)
self.assertIs(user_profile, None)
@override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',)) @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',))
def test_login_success_with_valid_subdomain(self) -> None: def test_login_success_with_valid_subdomain(self) -> None:
self.mock_ldap.directory = { self.mock_ldap.directory = {
@@ -2764,7 +2757,8 @@ 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') # type: ignore # complains that the function does not return any value! result = backend.authenticate(self.example_email("hamlet"), 'testing',
realm=get_realm('zulip'))
self.assertIs(result, None) self.assertIs(result, None)
def perform_ldap_sync(self, user_profile: UserProfile) -> None: def perform_ldap_sync(self, user_profile: UserProfile) -> None:

9
zerver/views/auth.py
View File

@@ -261,12 +261,11 @@ def remote_user_sso(request: HttpRequest,
subdomain = get_subdomain(request) subdomain = get_subdomain(request)
try: try:
realm = get_realm(subdomain) # type: Optional[Realm] realm = get_realm(subdomain)
except Realm.DoesNotExist: except Realm.DoesNotExist:
realm = None user_profile = None
# Since RemoteUserBackend will return None if Realm is None, we else:
# don't need to check whether `realm` is None. user_profile = authenticate(remote_user=remote_user, realm=realm)
user_profile = authenticate(remote_user=remote_user, realm=realm)
redirect_to = request.GET.get('next', '') redirect_to = request.GET.get('next', '')

38
zproject/backends.py
View File

@@ -154,13 +154,10 @@ 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: Optional[str]=None, realm: Optional[Realm]=None, 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:
# These are kwargs only for readability; they should never be None
assert username is not None
assert realm is not None
return common_get_active_user(username, realm, return_data) return common_get_active_user(username, realm, return_data)
return None return None
@@ -171,17 +168,10 @@ 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: Optional[str]=None, password: Optional[str]=None, def authenticate(self, username: str, password: str,
realm: Optional[Realm]=None, 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. """
if username is None or password is None:
# Because of how we structure our auth calls to always
# specify which backend to use when not using
# EmailAuthBackend, username and password should always be set.
raise AssertionError("Invalid call to authenticate for EmailAuthBackend")
if realm is None:
return None
if not password_auth_enabled(realm): if not password_auth_enabled(realm):
if return_data is not None: if return_data is not None:
return_data['password_auth_disabled'] = True return_data['password_auth_disabled'] = True
@@ -211,15 +201,13 @@ 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: Optional[str]=None, realm: Optional[Realm]=None, 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
# used in this one code path and takes 30-50ms to import. # used in this one code path and takes 30-50ms to import.
from apiclient.sample_tools import client as googleapiclient from apiclient.sample_tools import client as googleapiclient
from oauth2client.crypt import AppIdentityError from oauth2client.crypt import AppIdentityError
if realm is None:
return None
if return_data is None: if return_data is None:
return_data = {} return_data = {}
@@ -250,11 +238,8 @@ class ZulipRemoteUserBackend(RemoteUserBackend):
""" """
create_unknown_user = False create_unknown_user = False
def authenticate(self, remote_user: Optional[str], realm: Optional[Realm]=None, 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]:
assert remote_user is not None
if realm is None:
return None
if not auth_enabled_helper(["RemoteUser"], realm): if not auth_enabled_helper(["RemoteUser"], realm):
return None return None
@@ -471,11 +456,9 @@ 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: Optional[Realm]=None, 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]:
if realm is None:
return None
self._realm = realm self._realm = realm
self._prereg_user = prereg_user self._prereg_user = prereg_user
if not ldap_auth_enabled(realm): if not ldap_auth_enabled(realm):
@@ -591,8 +574,8 @@ 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: Optional[Realm]=None, def authenticate(self, username: str, password: str, realm: Realm,
return_data: Optional[Dict[str, Any]]=None) -> None: return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]:
return None return None
def sync_user_from_ldap(user_profile: UserProfile) -> bool: def sync_user_from_ldap(user_profile: UserProfile) -> bool:
@@ -628,11 +611,8 @@ 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: Optional[str]=None, realm: Optional[Realm]=None, 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]:
assert dev_auth_username is not None
if realm is None:
return None
if not dev_auth_enabled(realm): if not dev_auth_enabled(realm):
return None return None
return common_get_active_user(dev_auth_username, realm, return_data=return_data) return common_get_active_user(dev_auth_username, realm, return_data=return_data)