From 737de6d4cdfb60c8278fad00735096821dad6d3c Mon Sep 17 00:00:00 2001
From: Alex Vandiver <alexmv@zulip.com>
Date: Tue, 28 Nov 2023 17:25:28 +0000
Subject: [PATCH] user_settings: Re-verify email addresses when enacting them.

---
 zerver/tests/test_email_change.py | 43 +++++++++++++++++++++++++
 zerver/views/user_settings.py     | 52 ++++++++++++++++---------------
 2 files changed, 70 insertions(+), 25 deletions(-)

diff --git a/zerver/tests/test_email_change.py b/zerver/tests/test_email_change.py
index 324429db18..e2b4d8f38c 100644
--- a/zerver/tests/test_email_change.py
+++ b/zerver/tests/test_email_change.py
@@ -241,6 +241,49 @@ class EmailChangeTestCase(ZulipTestCase):
         self.assertEqual(result.status_code, 400)
         self.assert_in_response("Already has an account", result)
 
+    def test_email_change_already_taken_later(self) -> None:
+        conflict_email = "conflict@zulip.com"
+        hamlet = self.example_user("hamlet")
+        cordelia = self.example_user("cordelia")
+
+        self.login_user(hamlet)
+        hamlet_url = self.generate_email_change_link(conflict_email)
+        self.logout()
+
+        self.login_user(cordelia)
+        cordelia_url = self.generate_email_change_link(conflict_email)
+        response = self.client_get(cordelia_url)
+        self.assertEqual(response.status_code, 200)
+        cordelia.refresh_from_db()
+        self.assertEqual(cordelia.delivery_email, conflict_email)
+
+        self.logout()
+        self.login_user(hamlet)
+        response = self.client_get(hamlet_url)
+        self.assertEqual(response.status_code, 400)
+        self.assert_in_response("Already has an account", response)
+
+    def test_change_email_to_disposable_email(self) -> None:
+        hamlet = self.example_user("hamlet")
+
+        # Make the realm allow permissive email changes, create a
+        # link, and then lock the permissions down -- the change
+        # should not be allowed when the link is followed.
+        realm = hamlet.realm
+        realm.disallow_disposable_email_addresses = False
+        realm.emails_restricted_to_domains = False
+        realm.save()
+
+        self.login_user(hamlet)
+        confirmation_url = self.generate_email_change_link("hamlet@mailnator.com")
+
+        realm.disallow_disposable_email_addresses = True
+        realm.save()
+
+        response = self.client_get(confirmation_url)
+        self.assertEqual(response.status_code, 400)
+        self.assert_in_response("Please use your real email address.", response)
+
     def test_unauthorized_email_change_from_email_confirmation_link(self) -> None:
         new_email = "hamlet-new@zulip.com"
         user_profile = self.example_user("hamlet")
diff --git a/zerver/views/user_settings.py b/zerver/views/user_settings.py
index 6f87c134cd..861b5eae0a 100644
--- a/zerver/views/user_settings.py
+++ b/zerver/views/user_settings.py
@@ -64,6 +64,31 @@ from zproject.backends import check_password_strength, email_belongs_to_ldap
 AVATAR_CHANGES_DISABLED_ERROR = gettext_lazy("Avatar changes are disabled in this organization.")
 
 
+def validate_email_change_request(user_profile: UserProfile, new_email: str) -> None:
+    if not user_profile.is_active:
+        # TODO: Make this into a user-facing error, not JSON
+        raise UserDeactivatedError
+
+    if user_profile.realm.email_changes_disabled and not user_profile.is_realm_admin:
+        raise JsonableError(_("Email address changes are disabled in this organization."))
+
+    error = validate_email_is_valid(
+        new_email,
+        get_realm_email_validator(user_profile.realm),
+    )
+    if error:
+        raise JsonableError(error)
+
+    try:
+        validate_email_not_already_in_realm(
+            user_profile.realm,
+            new_email,
+            verbose=False,
+        )
+    except ValidationError as e:
+        raise JsonableError(e.message)
+
+
 def confirm_email_change(request: HttpRequest, confirmation_key: str) -> HttpResponse:
     try:
         email_change_object = get_object_from_key(
@@ -91,13 +116,7 @@ def confirm_email_change(request: HttpRequest, confirmation_key: str) -> HttpRes
         if user_profile.realm.deactivated:
             return redirect_to_deactivation_notice()
 
-        if not user_profile.is_active:
-            # TODO: Make this into a user-facing error, not JSON
-            raise UserDeactivatedError
-
-        if user_profile.realm.email_changes_disabled and not user_profile.is_realm_admin:
-            raise JsonableError(_("Email address changes are disabled in this organization."))
-
+        validate_email_change_request(user_profile, new_email)
         do_change_user_delivery_email(user_profile, new_email)
 
     user_profile = UserProfile.objects.get(id=email_change_object.user_profile_id)
@@ -351,24 +370,7 @@ def json_change_settings(
     if email is not None:
         new_email = email.strip()
         if user_profile.delivery_email != new_email:
-            if user_profile.realm.email_changes_disabled and not user_profile.is_realm_admin:
-                raise JsonableError(_("Email address changes are disabled in this organization."))
-
-            error = validate_email_is_valid(
-                new_email,
-                get_realm_email_validator(user_profile.realm),
-            )
-            if error:
-                raise JsonableError(error)
-
-            try:
-                validate_email_not_already_in_realm(
-                    user_profile.realm,
-                    new_email,
-                    verbose=False,
-                )
-            except ValidationError as e:
-                raise JsonableError(e.message)
+            validate_email_change_request(user_profile, new_email)
 
             ratelimited, time_until_free = RateLimitedUser(
                 user_profile, domain="email_change_by_user"