ubernostrum / django-registration

An extensible user-registration app for Django.
https://django-registration.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
924 stars 240 forks source link

E-Mail's domain check is not so strict #248

Closed th3hamm0r closed 22 hours ago

th3hamm0r commented 4 days ago

With #238, django's default email validator has been removed. This is also documented in the code with: https://github.com/ubernostrum/django-registration/blob/d217015ddbfd0b679b6eb3eccec542ce0fd8ad8a/src/django_registration/forms.py#L254-L262

Actually, this is not completely true, as I've found out that django's default validator does not allow test@example or test@1234, but HTML5 (so the HTML5EmailValidator) does. Django basically only allows domains with a ".", except for "localhost" (see docs).

Since we don't want "localhost" domains as emails, we're currently using this custom version of the HTML5EmailValidator:

from django.core.validators import EmailValidator
from django.utils.deconstruct import deconstructible
from django_registration.validators import (
    HTML5EmailValidator as BaseHTML5EmailValidator,
)

@deconstructible
class HTML5EmailValidator(BaseHTML5EmailValidator):
    # By default, django does not allow domains without a ".", except "localhost"
    django_validator = EmailValidator(allowlist=[])

    def __call__(self, value):
        super().__call__(value)
        self.django_validator(value)

It won't produce duplicate error messages, Django's validation happens inside our validator.

Shouldn't we add this stricter validation as a default to django-registration too?

ubernostrum commented 3 days ago

I'm not sure I understand the report here. I added some tests without changing the regex, and it appears that test@example and test@1234 are both rejected (though test@localhost is allowed, similar to Django's default).

th3hamm0r commented 3 days ago

hm, that's strange. thx for the response. I will check that again next week 👍

ubernostrum commented 22 hours ago

If you can replicate the issue, feel free to reopen with more information.

th3hamm0r commented 19 hours ago

Fyi, I've found the issue in our code: We're using a custom EmailField (with dns validation and some other changes), and I think based on the comment that django-registration's email validation is significantly stricter than Django's, we've falsely assumed that we can just use the 2 validators (HTML5EmailValidator and validate_confusables_email) on the model field too... But actually, this is where Django's stricter domain validation kicks in: the RegistrationForm only uses the 2 validators, but the full_clean() call on the instance (called by _post_clean()) then triggers Django's default validator.