auth: Include user-input email in some error messages in the login form.

Fixes #13126.
This commit is contained in:
Mateusz Mandera 2021-08-23 15:14:05 +02:00 committed by Tim Abbott
parent fb3864ea3c
commit c5806d9728
7 changed files with 63 additions and 23 deletions

View File

@ -92,7 +92,7 @@ page can be easily identified in it's respective JavaScript file. -->
</div> </div>
{% endif %} {% endif %}
{% if is_deactivated %} {% if deactivated_account_error %}
<div class="alert"> <div class="alert">
{{ deactivated_account_error }} {{ deactivated_account_error }}
</div> </div>

View File

@ -49,12 +49,12 @@ MIT_VALIDATION_ERROR = (
+ '<a href="mailto:support@zulip.com">contact us</a>.' + '<a href="mailto:support@zulip.com">contact us</a>.'
) )
WRONG_SUBDOMAIN_ERROR = ( WRONG_SUBDOMAIN_ERROR = (
"Your Zulip account is not a member of the " "Your Zulip account {username} is not a member of the "
+ "organization associated with this subdomain. " + "organization associated with this subdomain. "
+ "Please contact your organization administrator with any questions." + "Please contact your organization administrator with any questions."
) )
DEACTIVATED_ACCOUNT_ERROR = ( DEACTIVATED_ACCOUNT_ERROR = (
"Your account is no longer active. " "Your account {username} is no longer active. "
+ "Please contact your organization administrator to reactivate it." + "Please contact your organization administrator to reactivate it."
) )
PASSWORD_RESET_NEEDED_ERROR = ( PASSWORD_RESET_NEEDED_ERROR = (
@ -432,13 +432,15 @@ class OurAuthenticationForm(AuthenticationForm):
# We exclude mirror dummy accounts here. They should be treated as the # 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 # user never having had an account, so we let them fall through to the
# normal invalid_login case below. # normal invalid_login case below.
raise ValidationError(mark_safe(DEACTIVATED_ACCOUNT_ERROR)) error_message = DEACTIVATED_ACCOUNT_ERROR.format(username=username)
raise ValidationError(mark_safe(error_message))
if return_data.get("invalid_subdomain"): if return_data.get("invalid_subdomain"):
logging.warning( logging.warning(
"User %s attempted password login to wrong subdomain %s", username, subdomain "User %s attempted password login to wrong subdomain %s", username, subdomain
) )
raise ValidationError(mark_safe(WRONG_SUBDOMAIN_ERROR)) error_message = WRONG_SUBDOMAIN_ERROR.format(username=username)
raise ValidationError(mark_safe(error_message))
if self.user_cache is None: if self.user_cache is None:
raise forms.ValidationError( raise forms.ValidationError(

View File

@ -168,7 +168,10 @@ class AuthBackendTest(ZulipTestCase):
if isinstance(backend, SocialAuthMixin): if isinstance(backend, SocialAuthMixin):
# Returns a redirect to login page with an error. # Returns a redirect to login page with an error.
self.assertEqual(result.status_code, 302) self.assertEqual(result.status_code, 302)
self.assertEqual(result.url, user_profile.realm.uri + "/login/?is_deactivated=true") self.assertEqual(
result.url,
f"{user_profile.realm.uri}/login/?is_deactivated={user_profile.delivery_email}",
)
else: else:
# Just takes you back to the login page treating as # Just takes you back to the login page treating as
# invalid auth; this is correct because the form will # invalid auth; this is correct because the form will
@ -1098,7 +1101,10 @@ class SocialAuthBase(DesktopFlowTestingLib, ZulipTestCase):
account_data_dict, expect_choose_email_screen=True, subdomain="zulip" account_data_dict, expect_choose_email_screen=True, subdomain="zulip"
) )
self.assertEqual(result.status_code, 302) self.assertEqual(result.status_code, 302)
self.assertEqual(result.url, user_profile.realm.uri + "/login/?is_deactivated=true") self.assertEqual(
result.url,
f"{user_profile.realm.uri}/login/?is_deactivated={user_profile.delivery_email}",
)
self.assertEqual( self.assertEqual(
m.output, m.output,
[ [
@ -1107,7 +1113,11 @@ class SocialAuthBase(DesktopFlowTestingLib, ZulipTestCase):
) )
], ],
) )
# TODO: verify whether we provide a clear error message
result = self.client_get(result.url)
self.assert_in_success_response(
[f"Your account {user_profile.delivery_email} is no longer active."], result
)
def test_social_auth_invalid_realm(self) -> None: def test_social_auth_invalid_realm(self) -> None:
account_data_dict = self.get_account_data_dict(email=self.email, name=self.name) account_data_dict = self.get_account_data_dict(email=self.email, name=self.name)

View File

@ -1218,8 +1218,10 @@ class InactiveUserTest(ZulipTestCase):
user_profile = self.example_user("hamlet") user_profile = self.example_user("hamlet")
do_deactivate_user(user_profile, acting_user=None) do_deactivate_user(user_profile, acting_user=None)
result = self.login_with_return(self.example_email("hamlet")) result = self.login_with_return(user_profile.delivery_email)
self.assert_in_response("Your account is no longer active.", result) self.assert_in_response(
"Your account {} is no longer active.".format(user_profile.delivery_email), result
)
def test_login_deactivated_mirror_dummy(self) -> None: def test_login_deactivated_mirror_dummy(self) -> None:
""" """
@ -1260,7 +1262,10 @@ class InactiveUserTest(ZulipTestCase):
form = OurAuthenticationForm(request, payload) form = OurAuthenticationForm(request, payload)
with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.EmailAuthBackend",)): with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.EmailAuthBackend",)):
self.assertFalse(form.is_valid()) self.assertFalse(form.is_valid())
self.assertIn("Your account is no longer active", str(form.errors)) self.assertIn(
"Your account {} is no longer active".format(user_profile.delivery_email),
str(form.errors),
)
def test_webhook_deactivated_user(self) -> None: def test_webhook_deactivated_user(self) -> None:
""" """

View File

@ -735,9 +735,11 @@ class LoginTest(ZulipTestCase):
def test_login_deactivated_user(self) -> None: def test_login_deactivated_user(self) -> None:
user_profile = self.example_user("hamlet") user_profile = self.example_user("hamlet")
do_deactivate_user(user_profile, acting_user=None) do_deactivate_user(user_profile, acting_user=None)
result = self.login_with_return(self.example_email("hamlet"), "xxx") result = self.login_with_return(user_profile.delivery_email, "xxx")
self.assertEqual(result.status_code, 200) self.assertEqual(result.status_code, 200)
self.assert_in_response("Your account is no longer active.", result) self.assert_in_response(
"Your account {} is no longer active.".format(user_profile.delivery_email), result
)
self.assert_logged_in_user_id(None) self.assert_logged_in_user_id(None)
def test_login_bad_password(self) -> None: def test_login_bad_password(self) -> None:
@ -809,8 +811,9 @@ class LoginTest(ZulipTestCase):
self.assert_logged_in_user_id(None) self.assert_logged_in_user_id(None)
def test_login_wrong_subdomain(self) -> None: def test_login_wrong_subdomain(self) -> None:
email = self.mit_email("sipbtest")
with self.assertLogs(level="WARNING") as m: with self.assertLogs(level="WARNING") as m:
result = self.login_with_return(self.mit_email("sipbtest"), "xxx") result = self.login_with_return(email, "xxx")
self.assertEqual( self.assertEqual(
m.output, m.output,
[ [
@ -818,11 +821,11 @@ class LoginTest(ZulipTestCase):
], ],
) )
self.assertEqual(result.status_code, 200) self.assertEqual(result.status_code, 200)
self.assert_in_response( expected_error = (
"Your Zulip account is not a member of the " f"Your Zulip account {email} is not a member of the "
"organization associated with this subdomain.", + "organization associated with this subdomain."
result,
) )
self.assert_in_response(expected_error, result)
self.assert_logged_in_user_id(None) self.assert_logged_in_user_id(None)
def test_login_invalid_subdomain(self) -> None: def test_login_invalid_subdomain(self) -> None:
@ -5311,6 +5314,12 @@ class TestLoginPage(ZulipTestCase):
self.assertEqual(result.status_code, 400) self.assertEqual(result.status_code, 400)
self.assert_in_response("Authentication subdomain", result) self.assert_in_response("Authentication subdomain", result)
def test_login_page_is_deactivated_validation(self) -> None:
with patch("zerver.views.auth.logging.info") as mock_info:
result = self.client_get("/login/?is_deactivated=invalid_email")
mock_info.assert_called_once()
self.assert_not_in_success_response(["invalid_email"], result)
class TestFindMyTeam(ZulipTestCase): class TestFindMyTeam(ZulipTestCase):
def test_template(self) -> None: def test_template(self) -> None:

View File

@ -12,15 +12,19 @@ from django.contrib.auth import authenticate
from django.contrib.auth.views import LoginView as DjangoLoginView from django.contrib.auth.views import LoginView as DjangoLoginView
from django.contrib.auth.views import PasswordResetView as DjangoPasswordResetView from django.contrib.auth.views import PasswordResetView as DjangoPasswordResetView
from django.contrib.auth.views import logout_then_login as django_logout_then_login from django.contrib.auth.views import logout_then_login as django_logout_then_login
from django.core.exceptions import ValidationError
from django.core.validators import validate_email
from django.forms import Form from django.forms import Form
from django.http import HttpRequest, HttpResponse, HttpResponseRedirect, HttpResponseServerError from django.http import HttpRequest, HttpResponse, HttpResponseRedirect, HttpResponseServerError
from django.shortcuts import redirect, render from django.shortcuts import redirect, render
from django.template.response import SimpleTemplateResponse from django.template.response import SimpleTemplateResponse
from django.urls import reverse from django.urls import reverse
from django.utils.html import escape
from django.utils.http import url_has_allowed_host_and_scheme from django.utils.http import url_has_allowed_host_and_scheme
from django.utils.translation import gettext as _ from django.utils.translation import gettext as _
from django.views.decorators.csrf import csrf_exempt from django.views.decorators.csrf import csrf_exempt
from django.views.decorators.http import require_safe from django.views.decorators.http import require_safe
from jinja2.utils import Markup as mark_safe
from social_django.utils import load_backend, load_strategy from social_django.utils import load_backend, load_strategy
from two_factor.forms import BackupTokenForm from two_factor.forms import BackupTokenForm
from two_factor.views import LoginView as BaseTwoFactorLoginView from two_factor.views import LoginView as BaseTwoFactorLoginView
@ -670,13 +674,22 @@ def redirect_to_deactivation_notice() -> HttpResponse:
def update_login_page_context(request: HttpRequest, context: Dict[str, Any]) -> None: def update_login_page_context(request: HttpRequest, context: Dict[str, Any]) -> None:
for key in ("email", "already_registered", "is_deactivated"): for key in ("email", "already_registered"):
try: try:
context[key] = request.GET[key] context[key] = request.GET[key]
except KeyError: except KeyError:
pass pass
context["deactivated_account_error"] = DEACTIVATED_ACCOUNT_ERROR deactivated_email = request.GET.get("is_deactivated")
if deactivated_email is None:
return
try:
validate_email(deactivated_email)
context["deactivated_account_error"] = mark_safe(
DEACTIVATED_ACCOUNT_ERROR.format(username=escape(deactivated_email))
)
except ValidationError:
logging.info("Invalid email in is_deactivated param to login page: %s", deactivated_email)
class TwoFactorLoginView(BaseTwoFactorLoginView): class TwoFactorLoginView(BaseTwoFactorLoginView):

View File

@ -73,6 +73,7 @@ from zerver.lib.rate_limiter import RateLimitedObject
from zerver.lib.redis_utils import get_dict_from_redis, get_redis_client, put_dict_in_redis from zerver.lib.redis_utils import get_dict_from_redis, get_redis_client, put_dict_in_redis
from zerver.lib.request import RequestNotes from zerver.lib.request import RequestNotes
from zerver.lib.subdomains import get_subdomain from zerver.lib.subdomains import get_subdomain
from zerver.lib.url_encoding import add_query_to_redirect_url
from zerver.lib.users import check_full_name, validate_user_custom_profile_field from zerver.lib.users import check_full_name, validate_user_custom_profile_field
from zerver.models import ( from zerver.models import (
CustomProfileField, CustomProfileField,
@ -1346,11 +1347,11 @@ def redirect_to_login(realm: Realm) -> HttpResponseRedirect:
return HttpResponseRedirect(redirect_url) return HttpResponseRedirect(redirect_url)
def redirect_deactivated_user_to_login(realm: Realm) -> HttpResponseRedirect: def redirect_deactivated_user_to_login(realm: Realm, email: str) -> HttpResponseRedirect:
# Specifying the template name makes sure that the user is not redirected to dev_login in case of # Specifying the template name makes sure that the user is not redirected to dev_login in case of
# a deactivated account on a test server. # a deactivated account on a test server.
login_url = reverse("login_page", kwargs={"template_name": "zerver/login.html"}) login_url = reverse("login_page", kwargs={"template_name": "zerver/login.html"})
redirect_url = realm.uri + login_url + "?is_deactivated=true" redirect_url = add_query_to_redirect_url(realm.uri + login_url, f"is_deactivated={email}")
return HttpResponseRedirect(redirect_url) return HttpResponseRedirect(redirect_url)
@ -1565,7 +1566,7 @@ def social_auth_finish(
return_data["inactive_user_id"], return_data["inactive_user_id"],
return_data["realm_string_id"], return_data["realm_string_id"],
) )
return redirect_deactivated_user_to_login(realm) return redirect_deactivated_user_to_login(realm, return_data["validated_email"])
if auth_backend_disabled or inactive_realm or no_verified_email or email_not_associated: if auth_backend_disabled or inactive_realm or no_verified_email or email_not_associated:
# Redirect to login page. We can't send to registration # Redirect to login page. We can't send to registration