diff --git a/templates/zerver/login.html b/templates/zerver/login.html index 0e8da16a53..01ce53104e 100644 --- a/templates/zerver/login.html +++ b/templates/zerver/login.html @@ -92,7 +92,7 @@ page can be easily identified in it's respective JavaScript file. --> {% endif %} - {% if is_deactivated %} + {% if deactivated_account_error %}
{{ deactivated_account_error }}
diff --git a/zerver/forms.py b/zerver/forms.py index e0362228c8..f9c02ba2e1 100644 --- a/zerver/forms.py +++ b/zerver/forms.py @@ -49,12 +49,12 @@ MIT_VALIDATION_ERROR = ( + 'contact us.' ) 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. " + "Please contact your organization administrator with any questions." ) DEACTIVATED_ACCOUNT_ERROR = ( - "Your account is no longer active. " + "Your account {username} is no longer active. " + "Please contact your organization administrator to reactivate it." ) PASSWORD_RESET_NEEDED_ERROR = ( @@ -432,13 +432,15 @@ class OurAuthenticationForm(AuthenticationForm): # 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 # 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"): logging.warning( "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: raise forms.ValidationError( diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index e006060d0a..ceab456140 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -168,7 +168,10 @@ class AuthBackendTest(ZulipTestCase): if isinstance(backend, SocialAuthMixin): # Returns a redirect to login page with an error. 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: # Just takes you back to the login page treating as # 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" ) 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( 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: account_data_dict = self.get_account_data_dict(email=self.email, name=self.name) diff --git a/zerver/tests/test_decorators.py b/zerver/tests/test_decorators.py index 52ce09fd94..5228e1ee3b 100644 --- a/zerver/tests/test_decorators.py +++ b/zerver/tests/test_decorators.py @@ -1218,8 +1218,10 @@ class InactiveUserTest(ZulipTestCase): user_profile = self.example_user("hamlet") do_deactivate_user(user_profile, acting_user=None) - result = self.login_with_return(self.example_email("hamlet")) - self.assert_in_response("Your account is no longer active.", result) + result = self.login_with_return(user_profile.delivery_email) + self.assert_in_response( + "Your account {} is no longer active.".format(user_profile.delivery_email), result + ) def test_login_deactivated_mirror_dummy(self) -> None: """ @@ -1260,7 +1262,10 @@ class InactiveUserTest(ZulipTestCase): form = OurAuthenticationForm(request, payload) with self.settings(AUTHENTICATION_BACKENDS=("zproject.backends.EmailAuthBackend",)): 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: """ diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 356a734e42..8c0803f44a 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -735,9 +735,11 @@ class LoginTest(ZulipTestCase): def test_login_deactivated_user(self) -> None: user_profile = self.example_user("hamlet") 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.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) def test_login_bad_password(self) -> None: @@ -809,8 +811,9 @@ class LoginTest(ZulipTestCase): self.assert_logged_in_user_id(None) def test_login_wrong_subdomain(self) -> None: + email = self.mit_email("sipbtest") 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( m.output, [ @@ -818,11 +821,11 @@ class LoginTest(ZulipTestCase): ], ) self.assertEqual(result.status_code, 200) - self.assert_in_response( - "Your Zulip account is not a member of the " - "organization associated with this subdomain.", - result, + expected_error = ( + f"Your Zulip account {email} is not a member of the " + + "organization associated with this subdomain." ) + self.assert_in_response(expected_error, result) self.assert_logged_in_user_id(None) def test_login_invalid_subdomain(self) -> None: @@ -5311,6 +5314,12 @@ class TestLoginPage(ZulipTestCase): self.assertEqual(result.status_code, 400) 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): def test_template(self) -> None: diff --git a/zerver/views/auth.py b/zerver/views/auth.py index c3c0943b59..cd85e5ce8e 100644 --- a/zerver/views/auth.py +++ b/zerver/views/auth.py @@ -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 PasswordResetView as DjangoPasswordResetView 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.http import HttpRequest, HttpResponse, HttpResponseRedirect, HttpResponseServerError from django.shortcuts import redirect, render from django.template.response import SimpleTemplateResponse 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.translation import gettext as _ from django.views.decorators.csrf import csrf_exempt 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 two_factor.forms import BackupTokenForm 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: - for key in ("email", "already_registered", "is_deactivated"): + for key in ("email", "already_registered"): try: context[key] = request.GET[key] except KeyError: 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): diff --git a/zproject/backends.py b/zproject/backends.py index a3bd62261c..e103fab02b 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -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.request import RequestNotes 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.models import ( CustomProfileField, @@ -1346,11 +1347,11 @@ def redirect_to_login(realm: Realm) -> HttpResponseRedirect: 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 # a deactivated account on a test server. 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) @@ -1565,7 +1566,7 @@ def social_auth_finish( return_data["inactive_user_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: # Redirect to login page. We can't send to registration