diff --git a/confirmation/models.py b/confirmation/models.py index 4081e80261..1640201488 100644 --- a/confirmation/models.py +++ b/confirmation/models.py @@ -170,9 +170,9 @@ class ConfirmationType: _properties = { - Confirmation.USER_REGISTRATION: ConfirmationType("check_prereg_key_and_redirect"), + Confirmation.USER_REGISTRATION: ConfirmationType("get_prereg_key_and_redirect"), Confirmation.INVITATION: ConfirmationType( - "check_prereg_key_and_redirect", validity_in_days=settings.INVITATION_LINK_VALIDITY_DAYS + "get_prereg_key_and_redirect", validity_in_days=settings.INVITATION_LINK_VALIDITY_DAYS ), Confirmation.EMAIL_CHANGE: ConfirmationType("confirm_email_change"), Confirmation.UNSUBSCRIBE: ConfirmationType( @@ -182,7 +182,7 @@ _properties = { Confirmation.MULTIUSE_INVITE: ConfirmationType( "join", validity_in_days=settings.INVITATION_LINK_VALIDITY_DAYS ), - Confirmation.REALM_CREATION: ConfirmationType("check_prereg_key_and_redirect"), + Confirmation.REALM_CREATION: ConfirmationType("get_prereg_key_and_redirect"), Confirmation.REALM_REACTIVATION: ConfirmationType("realm_reactivation"), } diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 1970d3d3d9..6e9d220fc9 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -855,7 +855,7 @@ class LoginTest(ZulipTestCase): with queries_captured() as queries, cache_tries_captured() as cache_tries: self.register(self.nonreg_email("test"), "test") # Ensure the number of queries we make is not O(streams) - self.assert_length(queries, 89) + self.assert_length(queries, 91) # We can probably avoid a couple cache hits here, but there doesn't # seem to be any O(N) behavior. Some of the cache hits are related @@ -2014,8 +2014,7 @@ so we didn't send them an invitation. We did send invitations to everyone else!" # Verify that using the wrong type doesn't work in the main confirm code path email_change_url = create_confirmation_link(prereg_user, Confirmation.EMAIL_CHANGE) email_change_key = email_change_url.split("/")[-1] - url = "/accounts/do_confirm/" + email_change_key - result = self.client_get(url) + result = self.client_post("/accounts/register/", {"key": email_change_key}) self.assertEqual(result.status_code, 404) self.assert_in_response( "Whoops. We couldn't find your confirmation link in the system.", result @@ -2032,8 +2031,17 @@ so we didn't send them an invitation. We did send invitations to everyone else!" with patch("confirmation.models.timezone_now", return_value=date_sent): url = create_confirmation_link(prereg_user, Confirmation.USER_REGISTRATION) - target_url = "/" + url.split("/", 3)[3] - result = self.client_get(target_url) + key = url.split("/")[-1] + confirmation_link_path = "/" + url.split("/", 3)[3] + # Both the confirmation link and submitting the key to the registration endpoint + # directly will return the appropriate error. + result = self.client_get(confirmation_link_path) + self.assertEqual(result.status_code, 404) + self.assert_in_response( + "Whoops. The confirmation link has expired or been deactivated.", result + ) + + result = self.client_post("/accounts/register/", {"key": key}) self.assertEqual(result.status_code, 404) self.assert_in_response( "Whoops. The confirmation link has expired or been deactivated.", result @@ -2124,7 +2132,9 @@ so we didn't send them an invitation. We did send invitations to everyone else!" url, {"key": registration_key, "from_confirmation": 1, "full_nme": "alice"} ) self.assertEqual(response.status_code, 404) - self.assert_in_response("The registration link has expired or is not valid.", response) + self.assert_in_response( + "Whoops. We couldn't find your confirmation link in the system.", response + ) registration_key = confirmation_link.split("/")[-1] response = self.client_post( diff --git a/zerver/views/registration.py b/zerver/views/registration.py index 22ab3c13f8..29c374cab1 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -1,6 +1,6 @@ import logging import urllib -from typing import Any, Dict, List, Optional +from typing import Any, Dict, List, Optional, Union from urllib.parse import urlencode from django.conf import settings @@ -94,10 +94,36 @@ if settings.BILLING_ENABLED: @has_request_variables -def check_prereg_key_and_redirect( +def get_prereg_key_and_redirect( request: HttpRequest, confirmation_key: str, full_name: Optional[str] = REQ(default=None) ) -> HttpResponse: - confirmation = Confirmation.objects.filter(confirmation_key=confirmation_key).first() + key_check_result = check_prereg_key(request, confirmation_key) + if isinstance(key_check_result, HttpResponse): + return key_check_result + # confirm_preregistrationuser.html just extracts the confirmation_key + # (and GET parameters) and redirects to /accounts/register, so that the + # user can enter their information on a cleaner URL. + return render( + request, + "confirmation/confirm_preregistrationuser.html", + context={"key": confirmation_key, "full_name": full_name}, + ) + + +def check_prereg_key( + request: HttpRequest, confirmation_key: str +) -> Union[Confirmation, HttpResponse]: + """ + Checks if the Confirmation key is valid, returning the Confirmation object in case of success + and an appropriate error page otherwise. + """ + try: + confirmation: Optional[Confirmation] = Confirmation.objects.get( + confirmation_key=confirmation_key + ) + except Confirmation.DoesNotExist: + confirmation = None + if confirmation is None or confirmation.type not in [ Confirmation.USER_REGISTRATION, Confirmation.INVITATION, @@ -117,14 +143,7 @@ def check_prereg_key_and_redirect( except ConfirmationKeyException as exception: return render_confirmation_key_error(request, exception) - # confirm_preregistrationuser.html just extracts the confirmation_key - # (and GET parameters) and redirects to /accounts/register, so that the - # user can enter their information on a cleaner URL. - return render( - request, - "confirmation/confirm_preregistrationuser.html", - context={"key": confirmation_key, "full_name": full_name}, - ) + return confirmation @require_post @@ -139,15 +158,12 @@ def accounts_register( default=None, converter=to_converted_or_fallback(to_non_negative_int, None) ), ) -> HttpResponse: - try: - confirmation = Confirmation.objects.get(confirmation_key=key) - except Confirmation.DoesNotExist: - return render(request, "zerver/confirmation_link_expired_error.html", status=404) + key_check_result = check_prereg_key(request, key) + if isinstance(key_check_result, HttpResponse): + return key_check_result - prereg_user = confirmation.content_object + prereg_user = key_check_result.content_object assert prereg_user is not None - if prereg_user.status == confirmation_settings.STATUS_REVOKED: - return render(request, "zerver/confirmation_link_expired_error.html", status=404) email = prereg_user.email realm_creation = prereg_user.realm_creation password_required = prereg_user.password_required diff --git a/zproject/urls.py b/zproject/urls.py index 780ae05f79..650da67f1f 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -128,9 +128,9 @@ from zerver.views.registration import ( accounts_home, accounts_home_from_multiuse_invite, accounts_register, - check_prereg_key_and_redirect, create_realm, find_account, + get_prereg_key_and_redirect, realm_redirect, ) from zerver.views.report import ( @@ -559,8 +559,8 @@ i18n_urls = [ path("accounts/register/", accounts_register, name="accounts_register"), path( "accounts/do_confirm/", - check_prereg_key_and_redirect, - name="check_prereg_key_and_redirect", + get_prereg_key_and_redirect, + name="get_prereg_key_and_redirect", ), path( "accounts/confirm_new_email/",