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.
This commit is contained in:
Mateusz Mandera
2022-01-23 20:37:40 +01:00
committed by Alex Vandiver
parent b5c7a79bdf
commit 0c227217b2
4 changed files with 24 additions and 19 deletions

View File

@@ -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

View File

@@ -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,

View File

@@ -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

View File

@@ -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(