From 6f62c993a67bdbb17e55c1aaa7645dcc90a9b3dd Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Mon, 2 Mar 2020 15:07:19 +0000 Subject: [PATCH] refactor: Extract get_existing_user_errors. This is a prep commit that will allow us to more efficiently validate a bunch of emails in the invite UI. This commit does not yet change any behavior or performance. A secondary goal of this commit is to prepare us to eliminate some hackiness related to how we construct `ValidationError` exceptions. It preserves some quirks of the prior implementation: - the strings we decided to translate here appear haphazard (and often get ignored anyway) - we use `msg` in most codepaths, but use `code` for invites Right now we never actually call this with more than one email, but that will change soon. Note that part of the rationale for the inner method here is to avoid a test coverage bug with `continue` in loops. --- zerver/lib/actions.py | 33 ++++++++++--------- zerver/lib/email_validation.py | 59 +++++++++++++++++++++++++++++++++- 2 files changed, 75 insertions(+), 17 deletions(-) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 0a39fcb7ad..171205f73a 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -116,7 +116,8 @@ from zerver.models import Realm, RealmEmoji, Stream, UserProfile, UserActivity, from zerver.lib.alert_words import get_alert_word_automaton from zerver.lib.avatar import avatar_url, avatar_url_from_dict from zerver.lib.email_validation import get_realm_email_validator, \ - validate_email_is_valid + validate_email_is_valid, get_existing_user_errors, \ + email_reserved_for_system_bots_error from zerver.lib.stream_recipient import StreamRecipientMap from zerver.lib.validator import check_widget_content from zerver.lib.widget import do_widget_post_save_actions @@ -5027,7 +5028,7 @@ def do_send_confirmation_email(invitee: PreregistrationUser, def email_not_system_bot(email: str) -> None: if is_cross_realm_bot_email(email): - msg = '%s is reserved for system bots' % (email,) + msg = email_reserved_for_system_bots_error(email) code = msg raise ValidationError( msg, @@ -5036,22 +5037,22 @@ def email_not_system_bot(email: str) -> None: ) def validate_email_not_already_in_realm(target_realm: Realm, email: str) -> None: - email_not_system_bot(email) + ''' + NOTE: + Only use this to validate that a single email + is not already used in the realm. - try: - existing_user_profile = get_user_by_delivery_email(email, target_realm) - except UserProfile.DoesNotExist: - return + We should start using bulk_check_new_emails() + for any endpoint that takes multiple emails, + such as the "invite" interface. + ''' + error_dict = get_existing_user_errors(target_realm, {email}) - if existing_user_profile.is_active: - if existing_user_profile.is_mirror_dummy: - raise AssertionError("Mirror dummy user is already active!") - # Other users should not already exist at all. - raise ValidationError(_('%s already has an account') % - (email,), code = _("Already has an account."), params={'deactivated': False}) - elif not existing_user_profile.is_mirror_dummy: - raise ValidationError('The account for %s has been deactivated' % (email,), - code = _("Account has been deactivated."), params={'deactivated': True}) + # Loop through errors, the only key should be our email. + for key, error_info in error_dict.items(): + assert key == email + msg, code, deactivated = error_info + raise ValidationError(msg, code=code, params=dict(deactivated=deactivated)) def validate_email( user_profile: UserProfile, diff --git a/zerver/lib/email_validation.py b/zerver/lib/email_validation.py index 1beda7c9df..74d067b0d0 100644 --- a/zerver/lib/email_validation.py +++ b/zerver/lib/email_validation.py @@ -1,4 +1,4 @@ -from typing import Callable, Optional +from typing import Callable, Dict, Optional, Set, Tuple from django.core import validators from django.core.exceptions import ValidationError @@ -9,11 +9,14 @@ from zerver.lib.name_restrictions import is_disposable_domain from zerver.models import ( email_to_username, email_to_domain, + get_user_by_delivery_email, + is_cross_realm_bot_email, DisposableEmailError, DomainNotAllowedForRealmError, EmailContainsPlusError, Realm, RealmDomain, + UserProfile, ) def validate_disposable(email: str) -> None: @@ -108,3 +111,57 @@ def validate_email_is_valid( return _("Email addresses containing + are not allowed.") return None + +def email_reserved_for_system_bots_error(email: str) -> str: + return '%s is reserved for system bots' % (email,) + +def get_existing_user_errors( + target_realm: Realm, + emails: Set[str], +) -> Dict[str, Tuple[str, Optional[str], bool]]: + ''' + We use this function even for a list of one emails. + + It checks "new" emails to make sure that they don't + already exist. There's a bit of fiddly logic related + to cross-realm bots and mirror dummies too. + ''' + errors = {} # type: Dict[str, Tuple[str, Optional[str], bool]] + + def process_email(email: str) -> None: + if is_cross_realm_bot_email(email): + msg = email_reserved_for_system_bots_error(email) + code = msg + deactivated = False + errors[email] = (msg, code, deactivated) + return + + try: + existing_user_profile = get_user_by_delivery_email(email, target_realm) + except UserProfile.DoesNotExist: + # HAPPY PATH! Most people invite users that don't exist yet. + return + + if existing_user_profile.is_mirror_dummy: + if existing_user_profile.is_active: + raise AssertionError("Mirror dummy user is already active!") + return + + ''' + Email has already been taken by a "normal" user. + ''' + deactivated = not existing_user_profile.is_active + + if existing_user_profile.is_active: + msg = _('%s already has an account') % (email,) + code = _("Already has an account.") + else: + msg = 'The account for %s has been deactivated' % (email,) + code = _("Account has been deactivated.") + + errors[email] = (msg, code, deactivated) + + for email in emails: + process_email(email) + + return errors