From be216506a972faf4d63f8ec86598e20626693665 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Thu, 21 Apr 2016 12:07:43 -0700 Subject: [PATCH] Improve api_fetch_api_key error messages. Previously, api_fetch_api_key would not give clear error messages if password auth was disabled or the user's realm had been deactivated; additionally, the account disabled error stopped triggering when we moved the active account check into the auth decorators. --- zerver/tests/test_auth_backends.py | 39 ++++++++++++++++++++++++++++++ zerver/views/__init__.py | 10 +++++--- zproject/backends.py | 22 +++++++++++++---- 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 7d4a163388..9aab41bc53 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -7,6 +7,7 @@ import mock from zerver.lib.actions import do_deactivate_realm, do_deactivate_user, \ do_reactivate_realm, do_reactivate_user +from zerver.lib.initial_password import initial_password from zerver.lib.test_helpers import ( AuthedTestCase, ) @@ -139,3 +140,41 @@ class AuthBackendTest(TestCase): with self.settings(SSO_APPEND_DOMAIN='zulip.com'): self.verify_backend(ZulipRemoteUserBackend(), email_to_username=email_to_username) + +class FetchAPIKeyTest(AuthedTestCase): + def setUp(self): + self.email = "hamlet@zulip.com" + self.user_profile = get_user_profile_by_email(self.email) + + def test_success(self): + result = self.client.post("/api/v1/fetch_api_key", + dict(username=self.email, + password=initial_password(self.email))) + self.assert_json_success(result) + + def test_wrong_password(self): + result = self.client.post("/api/v1/fetch_api_key", + dict(username=self.email, + password="wrong")) + self.assert_json_error(result, "Your username or password is incorrect.", 403) + + def test_password_auth_disabled(self): + with mock.patch('zproject.backends.password_auth_enabled', return_value=False): + result = self.client.post("/api/v1/fetch_api_key", + dict(username=self.email, + password=initial_password(self.email))) + self.assert_json_error_contains(result, "Password auth is disabled", 403) + + def test_inactive_user(self): + do_deactivate_user(self.user_profile) + result = self.client.post("/api/v1/fetch_api_key", + dict(username=self.email, + password=initial_password(self.email))) + self.assert_json_error_contains(result, "Your account has been disabled", 403) + + def test_deactivated_realm(self): + do_deactivate_realm(self.user_profile.realm) + result = self.client.post("/api/v1/fetch_api_key", + dict(username=self.email, + password=initial_password(self.email))) + self.assert_json_error_contains(result, "Your realm has been deactivated", 403) diff --git a/zerver/views/__init__.py b/zerver/views/__init__.py index c08eb9a347..3fb3faa75b 100644 --- a/zerver/views/__init__.py +++ b/zerver/views/__init__.py @@ -1078,14 +1078,18 @@ def api_fetch_api_key(request, username=REQ, password=REQ): if username == "google-oauth2-token": user_profile = authenticate(google_oauth2_token=password, return_data=return_data) else: - user_profile = authenticate(username=username, password=password) + user_profile = authenticate(username=username, password=password, return_data=return_data) + if return_data.get("inactive_user") == True: + return json_error("Your account has been disabled.", data={"reason": "user disable"}, status=403) + if return_data.get("inactive_realm") == True: + return json_error("Your realm has been deactivated.", data={"reason": "realm deactivated"}, status=403) + if return_data.get("password_auth_disabled") == True: + return json_error("Password auth is disabled in your team.", data={"reason": "password auth disabled"}, status=403) if user_profile is None: if return_data.get("valid_attestation") == True: # We can leak that the user is unregistered iff they present a valid authentication string for the user. return json_error("This user is not registered; do so from a browser.", data={"reason": "unregistered"}, status=403) return json_error("Your username or password is incorrect.", data={"reason": "incorrect_creds"}, status=403) - if not user_profile.is_active: - return json_error("Your account has been disabled.", data={"reason": "disabled"}, status=403) return json_success({"api_key": user_profile.api_key, "email": user_profile.email}) @authenticated_json_post_view diff --git a/zproject/backends.py b/zproject/backends.py index e9a2c05f25..e95e30daa9 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -40,12 +40,18 @@ def google_auth_enabled(): return True return False -def common_get_active_user_by_email(email): +def common_get_active_user_by_email(email, return_data=None): try: user_profile = get_user_profile_by_email(email) except UserProfile.DoesNotExist: return None - if not user_profile.is_active or user_profile.realm.deactivated: + if not user_profile.is_active: + if return_data is not None: + return_data['inactive_user'] = True + return None + if user_profile.realm.deactivated: + if return_data is not None: + return_data['inactive_realm'] = True return None return user_profile @@ -74,7 +80,7 @@ class EmailAuthBackend(ZulipAuthMixin): a username/password pair. """ - def authenticate(self, username=None, password=None): + def authenticate(self, username=None, password=None, return_data=None): """ Authenticate a user based on email address as the user name. """ if username is None or password is None: # Return immediately. Otherwise we will look for a SQL row with @@ -82,10 +88,12 @@ class EmailAuthBackend(ZulipAuthMixin): # exposure. return None - user_profile = common_get_active_user_by_email(username) + user_profile = common_get_active_user_by_email(username, return_data=return_data) if user_profile is None: return None if not password_auth_enabled(user_profile.realm): + if return_data is not None: + return_data['password_auth_disabled'] = True return None if user_profile.check_password(password): return user_profile @@ -112,7 +120,11 @@ class GoogleMobileOauth2Backend(ZulipAuthMixin): except UserProfile.DoesNotExist: return_data["valid_attestation"] = True return None - if not user_profile.is_active or user_profile.realm.deactivated: + if not user_profile.is_active: + return_data["inactive_user"] = True + return None + if user_profile.realm.deactivated: + return_data["inactive_realm"] = True return None return user_profile else: