create_realm: Refactor to deal ASAP with key record, not string.

Now, there's just one spot at the beginning of the function where we
inspect the string key the user gave us; and after that point, we not
only have validated that string but in fact are working from our own
record that it pointed to, not the string itself.

This simplifies the code a bit, e.g. by not repeatedly searching the
database for the key (and hoping everything agrees so that we keep
getting the same row), and it will simplify adding logic to inspect
row attributes like `presume_email_valid`.
This commit is contained in:
Greg Price 2018-01-29 11:54:49 -08:00 committed by Tim Abbott
parent f766ac6c10
commit 0dceeebd05
2 changed files with 21 additions and 11 deletions

View File

@ -132,13 +132,18 @@ _properties = {
# Arguably RealmCreationKey should just be another ConfirmationObjT and we should # Arguably RealmCreationKey should just be another ConfirmationObjT and we should
# add another Confirmation.type for this; it's this way for historical reasons. # add another Confirmation.type for this; it's this way for historical reasons.
def check_key_is_valid(creation_key: Text) -> bool: def validate_key(creation_key: Optional[str]) -> Optional['RealmCreationKey']:
if not RealmCreationKey.objects.filter(creation_key=creation_key).exists(): """Get the record for this key, raising InvalidCreationKey if non-None but invalid."""
return False if creation_key is None:
time_elapsed = timezone_now() - RealmCreationKey.objects.get(creation_key=creation_key).date_created return None
try:
key_record = RealmCreationKey.objects.get(creation_key=creation_key)
except RealmCreationKey.DoesNotExist:
raise RealmCreationKey.Invalid()
time_elapsed = timezone_now() - key_record.date_created
if time_elapsed.total_seconds() > settings.REALM_CREATION_LINK_VALIDITY_DAYS * 24 * 3600: if time_elapsed.total_seconds() > settings.REALM_CREATION_LINK_VALIDITY_DAYS * 24 * 3600:
return False raise RealmCreationKey.Invalid()
return True return key_record
def generate_realm_creation_url(by_admin: bool=False) -> Text: def generate_realm_creation_url(by_admin: bool=False) -> Text:
key = generate_key() key = generate_key()
@ -157,3 +162,6 @@ class RealmCreationKey(models.Model):
# True just if we should presume the email address the user enters # True just if we should presume the email address the user enters
# is theirs, and skip sending mail to it to confirm that. # is theirs, and skip sending mail to it to confirm that.
presume_email_valid = models.BooleanField(default=False) # type: bool presume_email_valid = models.BooleanField(default=False) # type: bool
class Invalid(Exception):
pass

View File

@ -37,7 +37,7 @@ from zerver.views.auth import create_preregistration_user, \
from zproject.backends import ldap_auth_enabled, password_auth_enabled, ZulipLDAPAuthBackend from zproject.backends import ldap_auth_enabled, password_auth_enabled, ZulipLDAPAuthBackend
from confirmation.models import Confirmation, RealmCreationKey, ConfirmationKeyException, \ from confirmation.models import Confirmation, RealmCreationKey, ConfirmationKeyException, \
check_key_is_valid, create_confirmation_link, get_object_from_key, \ validate_key, create_confirmation_link, get_object_from_key, \
render_confirmation_key_error render_confirmation_key_error
import logging import logging
@ -327,12 +327,14 @@ def redirect_to_email_login_url(email: str) -> HttpResponseRedirect:
return HttpResponseRedirect(redirect_url) return HttpResponseRedirect(redirect_url)
def create_realm(request: HttpRequest, creation_key: Optional[Text]=None) -> HttpResponse: def create_realm(request: HttpRequest, creation_key: Optional[Text]=None) -> HttpResponse:
if creation_key is not None and not check_key_is_valid(creation_key): try:
key_record = validate_key(creation_key)
except RealmCreationKey.Invalid:
return render(request, "zerver/realm_creation_failed.html", return render(request, "zerver/realm_creation_failed.html",
context={'message': _('The organization creation link has expired' context={'message': _('The organization creation link has expired'
' or is not valid.')}) ' or is not valid.')})
if not settings.OPEN_REALM_CREATION: if not settings.OPEN_REALM_CREATION:
if creation_key is None: if key_record is None:
return render(request, "zerver/realm_creation_failed.html", return render(request, "zerver/realm_creation_failed.html",
context={'message': _('New organization creation disabled.')}) context={'message': _('New organization creation disabled.')})
@ -349,8 +351,8 @@ def create_realm(request: HttpRequest, creation_key: Optional[Text]=None) -> Htt
logging.error('Error in create_realm: %s' % (str(e),)) logging.error('Error in create_realm: %s' % (str(e),))
return HttpResponseRedirect("/config-error/smtp") return HttpResponseRedirect("/config-error/smtp")
if creation_key is not None: if key_record is not None:
RealmCreationKey.objects.get(creation_key=creation_key).delete() key_record.delete()
return HttpResponseRedirect(reverse('send_confirm', kwargs={'email': email})) return HttpResponseRedirect(reverse('send_confirm', kwargs={'email': email}))
else: else:
form = RealmCreationForm() form = RealmCreationForm()