mirror of
https://github.com/zulip/zulip.git
synced 2025-10-23 04:52:12 +00:00
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.
This commit is contained in:
committed by
Tim Abbott
parent
00e0b63c6c
commit
4006bb5153
@@ -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
|
||||
|
@@ -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:
|
||||
|
@@ -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)
|
||||
|
@@ -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."),
|
||||
|
@@ -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
|
||||
|
Reference in New Issue
Block a user