From 802d3dbbf4b020885c308f584ae536b7c2bb137e Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Tue, 7 May 2019 17:18:34 -0700 Subject: [PATCH] authenticate: Use keyword-only parameters. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- zerver/tests/test_auth_backends.py | 38 +++++++++++++++--------------- zproject/backends.py | 14 +++++------ 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 89cd284815..42cb1c2dc5 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -178,7 +178,7 @@ class AuthBackendTest(ZulipTestCase): mock.patch('zproject.backends.password_auth_enabled', return_value=True): 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"), password=password, return_data=return_data) @@ -211,8 +211,8 @@ class AuthBackendTest(ZulipTestCase): user_profile.save() # Verify if a realm has password auth disabled, correct password is rejected with mock.patch('zproject.backends.password_auth_enabled', return_value=False): - self.assertIsNone(EmailAuthBackend().authenticate(self.example_email('hamlet'), - password, + self.assertIsNone(EmailAuthBackend().authenticate(username=self.example_email('hamlet'), + password=password, realm=get_realm("zulip"))) 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.attrs', 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'), ( mock.patch('django_auth_ldap.backend._LDAPUser._check_requirements')), ( @@ -412,11 +412,11 @@ class AuthBackendTest(ZulipTestCase): } } # type: Dict[str, Any] - def patched_authenticate(*args: Any, **kwargs: Any) -> Any: + def patched_authenticate(**kwargs: Any) -> Any: if 'subdomain' in kwargs: backend.strategy.session_set("subdomain", kwargs["subdomain"]) del kwargs['subdomain'] - result = orig_authenticate(backend, *args, **kwargs) + result = orig_authenticate(backend, **kwargs) return result for backend_name in backends_to_test: @@ -2406,7 +2406,7 @@ class TestLDAP(ZulipLDAPTestCase): 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('ldapuser1', 'dapu', + user_profile = self.backend.authenticate(username='ldapuser1', password='dapu', realm=get_realm('zulip')) assert(user_profile is None) @@ -2422,7 +2422,7 @@ class TestLDAP(ZulipLDAPTestCase): 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', + user_profile = self.backend.authenticate(username=self.example_email("hamlet"), password='testing', realm=get_realm('zulip')) assert(user_profile is not None) @@ -2439,7 +2439,7 @@ class TestLDAP(ZulipLDAPTestCase): 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("hamlet", 'testing', + user_profile = self.backend.authenticate(username="hamlet", password='testing', realm=get_realm('zulip')) assert(user_profile is not None) @@ -2456,7 +2456,7 @@ class TestLDAP(ZulipLDAPTestCase): with self.settings(LDAP_EMAIL_ATTR='email', AUTH_LDAP_BIND_PASSWORD='', 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')) assert (user_profile is not None) @@ -2473,7 +2473,7 @@ class TestLDAP(ZulipLDAPTestCase): LDAP_APPEND_DOMAIN='zulip.com', AUTH_LDAP_BIND_PASSWORD='', 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')) self.assertIs(user, None) @@ -2488,7 +2488,7 @@ class TestLDAP(ZulipLDAPTestCase): LDAP_APPEND_DOMAIN='zulip.com', AUTH_LDAP_BIND_PASSWORD='', 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')) self.assertIs(user, None) @@ -2645,7 +2645,7 @@ class TestLDAP(ZulipLDAPTestCase): @override_settings(AUTHENTICATION_BACKENDS=('zproject.backends.ZulipLDAPAuthBackend',)) def test_login_failure_when_domain_does_not_match(self) -> None: 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')) self.assertIs(user_profile, None) @@ -2666,7 +2666,7 @@ class TestLDAP(ZulipLDAPTestCase): AUTH_LDAP_BIND_PASSWORD='', AUTH_LDAP_USER_DN_TEMPLATE='uid=%(user)s,ou=users,dc=zulip,dc=com', 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')) self.assertEqual(user_profile.email, self.example_email('hamlet')) @@ -2681,7 +2681,7 @@ class TestLDAP(ZulipLDAPTestCase): 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', + user_profile = self.backend.authenticate(username=self.example_email("hamlet"), password='testing', realm=get_realm('zulip')) assert(user_profile is not None) self.assertEqual(user_profile.email, self.example_email("hamlet")) @@ -2699,7 +2699,7 @@ class TestLDAP(ZulipLDAPTestCase): 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', + user_profile = self.backend.authenticate(username=self.example_email("hamlet"), password='testing', realm=get_realm('zulip')) self.assertIs(user_profile, None) @@ -2721,7 +2721,7 @@ class TestLDAP(ZulipLDAPTestCase): LDAP_APPEND_DOMAIN='acme.com', AUTH_LDAP_BIND_PASSWORD='', 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')) assert(user_profile is not None) self.assertEqual(user_profile.email, 'nonexisting@acme.com') @@ -2747,7 +2747,7 @@ class TestLDAP(ZulipLDAPTestCase): AUTH_LDAP_BIND_PASSWORD='', 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'}): - user_profile = self.backend.authenticate('nonexisting@zulip.com', 'testing', + user_profile = self.backend.authenticate(username='nonexisting@zulip.com', password='testing', realm=get_realm('zulip')) assert(user_profile is not None) self.assertEqual(user_profile.email, 'nonexisting@zulip.com') @@ -2757,7 +2757,7 @@ class TestLDAP(ZulipLDAPTestCase): class TestZulipLDAPUserPopulator(ZulipLDAPTestCase): def test_authenticate(self) -> None: backend = ZulipLDAPUserPopulator() - result = backend.authenticate(self.example_email("hamlet"), 'testing', + result = backend.authenticate(username=self.example_email("hamlet"), password='testing', realm=get_realm('zulip')) self.assertIs(result, None) diff --git a/zproject/backends.py b/zproject/backends.py index 69368ef8a9..2b1ac3ced8 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -154,7 +154,7 @@ class ZulipDummyBackend(ZulipAuthMixin): 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, return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]: if use_dummy_backend: @@ -168,7 +168,7 @@ class EmailAuthBackend(ZulipAuthMixin): 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, return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]: """ 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 """ - 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]: # We lazily import apiclient as part of optimizing the base # import time for a Zulip management command, since it's only @@ -238,7 +238,7 @@ class ZulipRemoteUserBackend(RemoteUserBackend): """ 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]: if not auth_enabled_helper(["RemoteUser"], realm): return None @@ -456,7 +456,7 @@ class ZulipLDAPAuthBackendBase(ZulipAuthMixin, LDAPBackend): class ZulipLDAPAuthBackend(ZulipLDAPAuthBackendBase): 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, return_data: Optional[Dict[str, Any]]=None) -> Optional[UserProfile]: self._realm = realm @@ -574,7 +574,7 @@ class ZulipLDAPUserPopulator(ZulipLDAPAuthBackendBase): registration for organizations that use a different SSO solution 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 None @@ -611,7 +611,7 @@ def query_ldap(email: str) -> List[str]: class DevAuthBackend(ZulipAuthMixin): """Allow logging in as any user without a password. This is used for 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]: if not dev_auth_enabled(realm): return None