Skip to content

Commit

Permalink
Fix #238: Avoid duplicate validation of email address.
Browse files Browse the repository at this point in the history
The specific solution here is to apply only django-registration's own
validators, skipping Django's default email validator. Since
django-registration's validators are and always have been
significantly more restrictive, this does not change the set of
allowed email addreses.
  • Loading branch information
ubernostrum committed Jun 12, 2023
1 parent 0788474 commit 82cce46
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 21 deletions.
11 changes: 11 additions & 0 deletions docs/upgrade.rst
Expand Up @@ -20,6 +20,17 @@ django-registration 3.4
* The :ref:`reserved names list <reserved-names>` has a new entry: ``"xrpc"``,
which is used in domain-ownership verification by Bluesky/AT protocol.

* Validation of the email field in registration forms no longer applies
Django's default email validator, instead applying only django-registration's
:class:`~django_registration.validators.HTML5EmailValidator` and
:func:`~django_registration.validators.validate_confusables_email`. Since
django-registration's validators are significantly stricter, this does not
actually change the set of email addresses which will be accepted; all it
does is prevent a duplicate error message displaying when both the default
Django validator and the django-registration validators reject the email
address. See `GitHub issue #238
<https://github.com/ubernostrum/django-registration/issues/238>`_.

The supported Python and Django versions are changed to:

* Django 3.2, 4.1, and 4.2, on Python 3.7 (Django 3.2 only), 3.8, 3.9, 3.10,
Expand Down
2 changes: 1 addition & 1 deletion docs/validators.rst
Expand Up @@ -166,7 +166,7 @@ usernames and email addresses.
A custom validator which prohibits the use of dangerously-confusable email
address.

This validator will reject any email address where either the local-part of
This validator will reject any email address where either the local-part or
the domain is -- when considered in isolation -- dangerously confusable. A
string is dangerously confusable if it is a mixed-script value (as defined
by Unicode 'Script' property) which also contains one or more characters
Expand Down
12 changes: 9 additions & 3 deletions src/django_registration/forms.py
Expand Up @@ -58,9 +58,15 @@ def __init__(self, *args, **kwargs):
validators.validate_confusables,
]
self.fields[User.USERNAME_FIELD].validators.extend(username_validators)
self.fields[email_field].validators.extend(
(validators.HTML5EmailValidator(), validators.validate_confusables_email)
)
# django-registration's email validation is significantly stricter than Django's
# default email validation, which means that leaving Django's default validation
# on only causes confusion due to duplicate error messages (see GitHub issue
# #238). So we apply only the django-registration validators, not the default
# Django validator, on the email field.
self.fields[email_field].validators = [
validators.HTML5EmailValidator(),
validators.validate_confusables_email,
]
self.fields[email_field].required = True


Expand Down
44 changes: 27 additions & 17 deletions tests/test_forms.py
Expand Up @@ -34,12 +34,11 @@ def test_email_validation(self):
"""
Stricter-than-RFC email validation is applied.
This test is necessary because of the combination of
HTML5EmailValidator and validate_confusables_email() in the
default validator set for the email field of RegistrationForm;
some technically-valid email addresses which nonetheless
usually indicate bad faith or at least mischief are to be
rejected before the confusables validator is applied.
This test is necessary because of the combination of HTML5EmailValidator and
validate_confusables_email() in the default validator set for the email field of
RegistrationForm; some technically-valid email addresses which nonetheless
usually indicate bad faith or at least mischief are to be rejected before the
confusables validator is applied.
"""
user_model = get_user_model()
Expand Down Expand Up @@ -69,13 +68,27 @@ def test_email_validation(self):
in form.errors[user_model.get_email_field_name()]
)

def test_email_validated_once(self):
"""
GitHub issue #238: Django's default email validator (and its error message)
should not be applied to the email field of the registration form, since our
validators are stricter and use the same message (which can cause confusing
duplicate errors).
"""
user_model = get_user_model()
user_data = self.valid_data.copy()
user_data["email"] = "invalid_email"
form = forms.RegistrationForm(data=user_data)
email_field = user_model.get_email_field_name()
assert len(form.errors[email_field]) == 1

def test_username_uniqueness(self):
"""
Username uniqueness is enforced.
This test is necessary as of 2.1.x to ensure the base
UserCreationForm clean() continues to be called from the
overridden clean() in RegistrationForm.
This test is necessary as of 2.1.x to ensure the base UserCreationForm clean()
continues to be called from the overridden clean() in RegistrationForm.
"""
user_data = self.valid_data.copy()
Expand Down Expand Up @@ -107,8 +120,7 @@ def test_reserved_names(self):

def test_confusable_usernames(self):
"""
Usernames containing dangerously confusable use of Unicode are
disallowed.
Usernames containing dangerously confusable use of Unicode are disallowed.
"""
user_model = get_user_model()
Expand All @@ -126,8 +138,7 @@ def test_confusable_usernames(self):

def test_confusable_emails(self):
"""
Usernames containing dangerously confusable use of Unicode are
disallowed.
Usernames containing dangerously confusable use of Unicode are disallowed.
"""
for dangerous_value in (
Expand Down Expand Up @@ -172,8 +183,8 @@ class CustomReservedNamesForm(forms.RegistrationForm):

def test_reserved_name_non_string(self):
"""
GitHub issue #82: reserved-name validator should not attempt
to validate a non-string 'username'.
GitHub issue #82: reserved-name validator should not attempt to validate a
non-string 'username'.
"""
validator = validators.ReservedNameValidator()
Expand Down Expand Up @@ -310,8 +321,7 @@ def test_case_insensitive_form(self):

def test_tos_field(self):
"""
The terms-of-service field on RegistrationFormTermsOfService
is required.
The terms-of-service field on RegistrationFormTermsOfService is required.
"""
form = forms.RegistrationFormTermsOfService(data=self.valid_data.copy())
Expand Down

0 comments on commit 82cce46

Please sign in to comment.