From 4006bb5153b5c6fddb5d18bc4b9e3f702bb260aa Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Fri, 7 May 2021 15:10:35 +0200 Subject: [PATCH] auth: Improve display of errors when user needs to reset password. Raising jsonableError in the authentication form was non-ideal because it took the user to an ugly page with the returned json. We also add logging of this rare occurence of the scenario being handled here. --- zerver/forms.py | 7 +++++++ zerver/tests/test_auth_backends.py | 23 ++++++++++++++++++++++- zerver/tests/test_signup.py | 22 ++++++++++++++++++++++ zerver/views/auth.py | 6 ++++++ zproject/backends.py | 12 +++++++++++- 5 files changed, 68 insertions(+), 2 deletions(-) diff --git a/zerver/forms.py b/zerver/forms.py index f426444107..011d80c93a 100644 --- a/zerver/forms.py +++ b/zerver/forms.py @@ -53,6 +53,10 @@ DEACTIVATED_ACCOUNT_ERROR = ( "Your account is no longer active. " + "Please contact your organization administrator to reactivate it." ) +PASSWORD_RESET_NEEDED_ERROR = ( + "Your password has been disabled because it is too weak. " + "Reset your password to create a new one." +) PASSWORD_TOO_WEAK_ERROR = "The password is too weak." AUTHENTICATION_RATE_LIMITED_ERROR = ( "You're making too many attempts to sign in. " @@ -400,6 +404,9 @@ class OurAuthenticationForm(AuthenticationForm): if return_data.get("inactive_realm"): raise AssertionError("Programming error: inactive realm in authentication form") + if return_data.get("password_reset_needed"): + raise ValidationError(mark_safe(PASSWORD_RESET_NEEDED_ERROR)) + if return_data.get("inactive_user") and not return_data.get("is_mirror_dummy"): # We exclude mirror dummy accounts here. They should be treated as the # user never having had an account, so we let them fall through to the diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index f8a573e13d..418dd7d793 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -356,7 +356,7 @@ class AuthBackendTest(ZulipTestCase): "django.contrib.auth.hashers.PBKDF2PasswordHasher", ), PASSWORD_MIN_LENGTH=30, - ), self.assertRaises(JsonableError) as m: + ), self.assertLogs("zulip.auth.email", level="INFO"), self.assertRaises(JsonableError) as m: EmailAuthBackend().authenticate( request=mock.MagicMock(), username=self.example_email("hamlet"), @@ -3931,6 +3931,27 @@ class FetchAPIKeyTest(ZulipTestCase): ) self.assert_json_error_contains(result, "This organization has been deactivated", 403) + def test_old_weak_password_after_hasher_change(self) -> None: + user_profile = self.example_user("hamlet") + password = "a_password_of_22_chars" + + with self.settings(PASSWORD_HASHERS=("django.contrib.auth.hashers.PBKDF2PasswordHasher",)): + user_profile.set_password(password) + user_profile.save() + + with self.settings( + PASSWORD_HASHERS=( + "django.contrib.auth.hashers.Argon2PasswordHasher", + "django.contrib.auth.hashers.PBKDF2PasswordHasher", + ), + PASSWORD_MIN_LENGTH=30, + ), self.assertLogs("zulip.auth.email", level="INFO"): + result = self.client_post( + "/api/v1/fetch_api_key", + dict(username=self.email, password=password), + ) + self.assert_json_error(result, "You need to reset your password.", 403) + class DevFetchAPIKeyTest(ZulipTestCase): def setUp(self) -> None: diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index ee6b461c95..acbadc6ad1 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -706,6 +706,28 @@ class LoginTest(ZulipTestCase): remove_ratelimit_rule(10, 2, domain="authenticate_by_username") + def test_login_with_old_weak_password_after_hasher_change(self) -> None: + user_profile = self.example_user("hamlet") + password = "a_password_of_22_chars" + + with self.settings(PASSWORD_HASHERS=("django.contrib.auth.hashers.PBKDF2PasswordHasher",)): + user_profile.set_password(password) + user_profile.save() + + with self.settings( + PASSWORD_HASHERS=( + "django.contrib.auth.hashers.Argon2PasswordHasher", + "django.contrib.auth.hashers.PBKDF2PasswordHasher", + ), + PASSWORD_MIN_LENGTH=30, + ), self.assertLogs("zulip.auth.email", level="INFO"): + result = self.login_with_return(self.example_email("hamlet"), password) + self.assertEqual(result.status_code, 200) + self.assert_in_response( + "Your password has been disabled because it is too weak.", result + ) + self.assert_logged_in_user_id(None) + def test_login_nonexist_user(self) -> None: result = self.login_with_return("xxx@zulip.com", "xxx") self.assertEqual(result.status_code, 200) diff --git a/zerver/views/auth.py b/zerver/views/auth.py index ba9fc88d45..7ef4175177 100644 --- a/zerver/views/auth.py +++ b/zerver/views/auth.py @@ -831,6 +831,12 @@ def api_fetch_api_key( data={"reason": "password auth disabled"}, status=403, ) + if return_data.get("password_reset_needed"): + return json_error( + _("You need to reset your password."), + data={"reason": "password reset needed"}, + status=403, + ) if user_profile is None: return json_error( _("Your username or password is incorrect."), diff --git a/zproject/backends.py b/zproject/backends.py index ae218c61b8..2f4aba5de2 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -409,7 +409,17 @@ class EmailAuthBackend(ZulipAuthMixin): except PasswordTooWeakError: # In some rare cases when password hasher is changed and the user has # a weak password, PasswordTooWeakError will be raised. - raise JsonableError(_('You need to reset your password.')) + self.logger.info( + "User %s password can't be rehashed due to being too weak.", user_profile.id + ) + if return_data is not None: + return_data["password_reset_needed"] = True + return None + else: + # Since we can't communicate the situation via return_data, + # we have to raise an error - a silent failure would not be right + # because the password actually is correct, just can't be re-hashed. + raise JsonableError(_("You need to reset your password.")) if is_password_correct: return user_profile