From 0c227217b2b00e68ac6da6aa61c9d5256294d5fb Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Sun, 23 Jan 2022 20:37:40 +0100 Subject: [PATCH] registration: Change create_preregistration_user to take realm as arg. create_preregistration_user is a footgun, because it takes the realm from the request. The calling code is supposed to validate that registration for the realm is allowed first, but can sometimes do that on "realm" taken from something else than the request - and later on calls create_preregistration_user, thus leading to prereg user creation on unvalidated request.realm. It's safer, and makes more sense, for this function to take the intended realm as argument, instead of taking the entire request. It follows that the same should be done for prepare_activation_url. --- zerver/tests/test_signup.py | 2 +- zerver/views/auth.py | 14 +++++--------- zerver/views/development/registration.py | 8 ++++---- zerver/views/registration.py | 19 ++++++++++++++----- 4 files changed, 24 insertions(+), 19 deletions(-) diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 413932894c..a686334193 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -774,7 +774,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.assertEqual(len(queries), 72) + self.assertEqual(len(queries), 71) # 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 diff --git a/zerver/views/auth.py b/zerver/views/auth.py index 82651b199d..d749fe92b7 100644 --- a/zerver/views/auth.py +++ b/zerver/views/auth.py @@ -91,18 +91,14 @@ def get_safe_redirect_to(url: str, redirect_host: str) -> str: def create_preregistration_user( email: str, - request: HttpRequest, + realm: Optional[Realm], realm_creation: bool = False, password_required: bool = True, full_name: Optional[str] = None, full_name_validated: bool = False, -) -> HttpResponse: - realm = None - if not realm_creation: - try: - realm = get_realm(get_subdomain(request)) - except Realm.DoesNotExist: - pass +) -> PreregistrationUser: + assert not (realm_creation and realm is not None) + return PreregistrationUser.objects.create( email=email, realm_creation=realm_creation, @@ -202,7 +198,7 @@ def maybe_send_to_registration( except PreregistrationUser.DoesNotExist: prereg_user = create_preregistration_user( email, - request, + realm, password_required=password_required, full_name=full_name, full_name_validated=full_name_validated, diff --git a/zerver/views/development/registration.py b/zerver/views/development/registration.py index a41f4bf03c..97e1ba042e 100644 --- a/zerver/views/development/registration.py +++ b/zerver/views/development/registration.py @@ -5,6 +5,7 @@ from django.http import HttpRequest, HttpResponse from django.views.decorators.csrf import csrf_exempt from confirmation.models import Confirmation, create_confirmation_link +from zerver.context_processors import get_realm_from_request from zerver.lib.response import json_success from zerver.lib.subdomains import get_subdomain from zerver.models import UserProfile @@ -31,8 +32,9 @@ def register_development_user(request: HttpRequest) -> HttpResponse: count = UserProfile.objects.count() name = f"user-{count}" email = f"{name}@zulip.com" + realm = get_realm_from_request(request) prereg = create_preregistration_user( - email, request, realm_creation=False, password_required=False + email, realm, realm_creation=False, password_required=False ) activation_url = create_confirmation_link(prereg, Confirmation.USER_REGISTRATION) key = activation_url.split("/")[-1] @@ -48,9 +50,7 @@ def register_development_realm(request: HttpRequest) -> HttpResponse: name = f"user-{count}" email = f"{name}@zulip.com" realm_name = f"realm-{count}" - prereg = create_preregistration_user( - email, request, realm_creation=True, password_required=False - ) + prereg = create_preregistration_user(email, None, realm_creation=True, password_required=False) activation_url = create_confirmation_link(prereg, Confirmation.REALM_CREATION) key = activation_url.split("/")[-1] # Need to add test data to POST request as it doesn't originally contain the required parameters diff --git a/zerver/views/registration.py b/zerver/views/registration.py index 1fff3aea87..4804354b03 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -6,6 +6,7 @@ from urllib.parse import urlencode import pytz from django.conf import settings from django.contrib.auth import authenticate, get_backends +from django.contrib.sessions.models import Session from django.core import validators from django.core.exceptions import ValidationError from django.db.models import Q @@ -516,7 +517,9 @@ def login_and_go_to_home(request: HttpRequest, user_profile: UserProfile) -> Htt def prepare_activation_url( email: str, - request: HttpRequest, + session: Session, + *, + realm: Optional[Realm], realm_creation: bool = False, streams: Optional[List[Stream]] = None, invited_as: Optional[int] = None, @@ -525,7 +528,7 @@ def prepare_activation_url( Send an email with a confirmation link to the provided e-mail so the user can complete their registration. """ - prereg_user = create_preregistration_user(email, request, realm_creation) + prereg_user = create_preregistration_user(email, realm, realm_creation) if streams is not None: prereg_user.streams.set(streams) @@ -540,7 +543,7 @@ def prepare_activation_url( activation_url = create_confirmation_link(prereg_user, confirmation_type) if settings.DEVELOPMENT and realm_creation: - request.session["confirmation_key"] = {"confirmation_key": activation_url.split("/")[-1]} + session["confirmation_key"] = {"confirmation_key": activation_url.split("/")[-1]} return activation_url @@ -590,7 +593,9 @@ def create_realm(request: HttpRequest, creation_key: Optional[str] = None) -> Ht form = RealmCreationForm(request.POST) if form.is_valid(): email = form.cleaned_data["email"] - activation_url = prepare_activation_url(email, request, realm_creation=True) + activation_url = prepare_activation_url( + email, request.session, realm=None, realm_creation=True + ) if key_record is not None and key_record.presume_email_valid: # The user has a token created from the server command line; # skip confirming the email is theirs, taking their word for it. @@ -654,7 +659,11 @@ def accounts_home( return redirect_to_email_login_url(email) activation_url = prepare_activation_url( - email, request, streams=streams_to_subscribe, invited_as=invited_as + email, + request.session, + realm=realm, + streams=streams_to_subscribe, + invited_as=invited_as, ) try: send_confirm_registration_email(