From 0e2691815e2876c8fbb8a1be6a0b409ee0928405 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Mon, 25 Jul 2022 19:55:35 +0200 Subject: [PATCH] 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 | 8 ++++++++ zerver/tests/test_email_change.py | 20 ++++++++++++++++++++ zerver/views/registration.py | 7 ------- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/confirmation/models.py b/confirmation/models.py index add4fcec32..d1420e81d3 100644 --- a/confirmation/models.py +++ b/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. diff --git a/zerver/tests/test_email_change.py b/zerver/tests/test_email_change.py index fbdbca3bca..f69cea4d7c 100644 --- a/zerver/tests/test_email_change.py +++ b/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) diff --git a/zerver/views/registration.py b/zerver/views/registration.py index d1b0c71879..b86cb3eeab 100644 --- a/zerver/views/registration.py +++ b/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