ubernostrum / django-registration

An extensible user-registration app for Django.
BSD 3-Clause "New" or "Revised" License
919 stars 241 forks source link

Error importing RegistrationView when using custom user model with unique USERNAME_FIELD #234

Open kbarnes3 opened 2 years ago

kbarnes3 commented 2 years ago

I believe I'm hitting a bug where I can't import anything from django_registration.backends.activation.views because of a bug in django_registration.forms.RegistrationForm that is hit at import time.

Setup

Create a custom user model deriving from Django's AbstractBaseUser. Given it the following properties at a minimum:

    primary_email = models.EmailField(max_length=255, unique=True)
    USERNAME_FIELD = 'primary_email'
    EMAIL_FIELD = 'primary_email'

In your settings.py, set AUTH_USER_MODEL = 'users.MyUser'

In on of your urls.py, add this import: from django_registration.backends.activation.views import RegistrationView

This will lead to the following crash when starting Django:

 File "root\users\urls.py", line 3, in <module>
    from django_registration.backends.activation.views import RegistrationView
  File "root\venv\lib\site-packages\django_registration\backends\activation\views.py", line 18, in <module>
    from django_registration.views import ActivationView as BaseActivationView
  File "root\venv\lib\site-packages\django_registration\views.py", line 18, in <module>
    from .forms import RegistrationForm
  File "root\venv\lib\site-packages\django_registration\forms.py", line 22, in <module>
    class RegistrationForm(UserCreationForm):
  File "root\venv\lib\site-packages\django\forms\models.py", line 327, in __new__
    raise FieldError(message)
django.core.exceptions.FieldError: Unknown field(s) (primary_email) specified for User

Since this bug in RegistrationForm is happening at import time (and it is imported by RegistrationView), it doesn't matter that I'm following the documentation for custom user models and providing my own RegistrationForm - Django is crashing too early.

I believe the bug is around RegistrationForm's meta class:

    class Meta(UserCreationForm.Meta):
        fields = [
            User.USERNAME_FIELD,
            User.get_email_field_name(),
            "password1",
            "password2",
        ]

Earlier in the file, we have User = get_user_model(), which resolves to 'users.MyUser' in this case, with User.USERNAME_FIELD and User.get_email_field_name() both evaluating to 'primary_email'. However, RegistrationForm's meta class does not override UserCreateForm.Meta's model property, which is set to the Django default User model. This User model does not have a primary_email property, leading to the above error.

I believe the fix is to set hard code User to django.contrib.auth.models.User in forms.py. This would be a breaking change however, since people with custom classes would need to provide both model and fields in their RegistrationForm's Meta class.

ubernostrum commented 2 years ago

I need to think about this one.

kbarnes3 commented 2 years ago

I was able to work around this by setting my EmailField to be named username. This wasn't too difficult since I'm trying to incorporate Django-Registration into a new project that hasn't been deployed yet, but I imagine it would be more difficult for established sites to do this.

ubernostrum commented 2 years ago

The thing that's worrying me here is that there's no such thing as a truly generic user-registration view/form for Django. The amount of stuff you can do in a custom user model just makes it impossible to write a single form that can handle anything someone might do. And the documentation on using a custom user model with django-registration already makes clear that there are times when you're going to have to write your own stuff rather than be able to use simple subclasses of the built-in forms/views.

So I'm leaning toward just doing a better job of documenting what django-registration can support, and maybe better documentation of what you need to do when your user model gets too different from that.

kbarnes3 commented 2 years ago

I think a big problem with this issue is that the errors occur on import time. As it is structured now, not only do I have to write my own form, but I can't use the RegistrationView (because I can't import it) nor the URLs (because those import the RegistrationView as well). It would be nice if these errors could be deferred in some way to when the impacted classes are instantiated or something similar.

confuzeus commented 1 year ago

Why not use get_user_model in the RegistrationForm? I just submitted a PR for that but don't know if I'm missing a case where not using Django's default might break someone's code.

ubernostrum commented 1 year ago

Yeah, the issue is that UserCreationForm has defaulted to the built-in User forever, and I don't even know how much stuff depends on it.

If it's going to change, it'll have to change in a major version bump. I'm thinking through whether I want to do that to cover this case, since it's one that comes up but not super often, and the case of differing significantly from Django's built-in model is already one that has a lot of warnings in the docs.

ubernostrum commented 1 year ago

I've thought about this a lot, and I think the only way forward is to finally just give up on using Django's UserCreationForm.

That's a backwards-incompatible change, so I can't do it immediately, but once I get a release out for official Django 4.2 compatibility I'll start on a major version bump branch that will ship its own base form that doesn't hard-wire in the default Django User model.

ubernostrum commented 1 month ago

WIP of the rewrite of RegistrationForm is in this branch: https://github.com/ubernostrum/django-registration/tree/remove-usercreationform