confirmation: Prevent re-use of email change links.

The .status value of EmailChangeStatus was not being looked
at anywhere to prevent re-use of email change confirmation links. This
is not a security issue, since the EmailChangeStatus object has a fixed
value for the new_email, while the confirmation link has expiry time of
1 day, which prevents any reasonable malicious scenarios.

We fix this by making get_object_from_key look at
confirmation.content_object.status - which applies
generally to all confirmations where the attached object has the .status
attribute. This is desired, because we never want to
successfully get_object_from_key an object that has already been used or
reused.
This makes the prereg_user.status check in check_prereg_key redundant so
it can be deleted.

confirmation/models.py

@@ -17,6 +17,7 @@ from django.shortcuts import render
 from django.urls import reverse
 from django.utils.timezone import now as timezone_now
 
+from confirmation import settings as confirmation_settings
 from zerver.lib.types import UnspecifiedValue
 from zerver.models import EmailChangeStatus, MultiuseInvite, PreregistrationUser, Realm, UserProfile
 
@@ -77,6 +78,13 @@ def get_object_from_key(
     obj = confirmation.content_object
     assert obj is not None
 
+    used_value = confirmation_settings.STATUS_USED
+    revoked_value = confirmation_settings.STATUS_REVOKED
+    if hasattr(obj, "status") and obj.status in [used_value, revoked_value]:
+        # Confirmations where the object has the status attribute are one-time use
+        # and are marked after being used (or revoked).
+        raise ConfirmationKeyException(ConfirmationKeyException.EXPIRED)
+
     if mark_object_used:
         # MultiuseInvite objects have no status field, since they are
         # intended to be used more than once.

zerver/tests/test_email_change.py

@@ -11,6 +11,7 @@ from confirmation.models import (
     create_confirmation_link,
     generate_key,
 )
+from zerver.actions.create_user import do_reactivate_user
 from zerver.actions.realm_settings import do_deactivate_realm, do_set_realm_property
 from zerver.actions.user_settings import do_start_email_change_process
 from zerver.actions.users import do_deactivate_user
@@ -115,6 +116,21 @@ class EmailChangeTestCase(ZulipTestCase):
         obj.refresh_from_db()
         self.assertEqual(obj.status, 1)
 
+    def test_change_email_link_cant_be_reused(self) -> None:
+        new_email = "hamlet-new@zulip.com"
+        user_profile = self.example_user("hamlet")
+        self.login_user(user_profile)
+
+        activation_url = self.generate_email_change_link(new_email)
+        response = self.client_get(activation_url)
+        self.assertEqual(response.status_code, 200)
+
+        user_profile.refresh_from_db()
+        self.assertEqual(user_profile.delivery_email, new_email)
+
+        response = self.client_get(activation_url)
+        self.assertEqual(response.status_code, 404)
+
     def test_change_email_deactivated_user_realm(self) -> None:
         new_email = "hamlet-new@zulip.com"
         user_profile = self.example_user("hamlet")
@@ -126,6 +142,10 @@ class EmailChangeTestCase(ZulipTestCase):
         response = self.client_get(activation_url)
         self.assertEqual(response.status_code, 401)
 
+        do_reactivate_user(user_profile, acting_user=None)
+        self.login_user(user_profile)
+        activation_url = self.generate_email_change_link(new_email)
+
         do_deactivate_realm(user_profile.realm, acting_user=None)
 
         response = self.client_get(activation_url)

zerver/views/registration.py

@@ -16,7 +16,6 @@ from django.utils.translation import get_language
 from django.utils.translation import gettext as _
 from django_auth_ldap.backend import LDAPBackend, _LDAPUser
 
-from confirmation import settings as confirmation_settings
 from confirmation.models import (
     Confirmation,
     ConfirmationKeyException,
@@ -139,12 +138,6 @@ def check_prereg_key(request: HttpRequest, confirmation_key: str) -> Preregistra
     prereg_user = get_object_from_key(confirmation_key, confirmation_types, mark_object_used=False)
     assert isinstance(prereg_user, PreregistrationUser)
 
-    if prereg_user.status in [
-        confirmation_settings.STATUS_REVOKED,
-        confirmation_settings.STATUS_USED,
-    ]:
-        raise ConfirmationKeyException(ConfirmationKeyException.EXPIRED)
-
     # Defensive assert to make sure no mix-up in how .status is set leading to re-use
     # of a PreregistrationUser object.
     assert prereg_user.created_user is None