Opened 2 years ago

Closed 15 months ago

Last modified 12 months ago

#20086 closed Bug (fixed)

UserCreationForm does not support custom models.

Reported by: efrinut@… Owned by: nobody
Component: contrib.auth Version: 1.5
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hey,

It's my first ticket :)

I've found out that UserCreationForm() located in django\contrib\auth\forms.py does not support custom models.

In some places there should be used get_user_model() in my opinion.

    class Meta:
        model = User # Should be get_user_model()
        fields = ("username",)

    def clean_username(self):
        # Since User.username is unique, this check is redundant,
        # but it sets a nicer error message than the ORM. See #13147.
        username = self.cleaned_data["username"]
        try:
            User._default_manager.get(username=username) #Should be get_user_model()
        except User.DoesNotExist:
            return username
        raise forms.ValidationError(self.error_messages['duplicate_username'])



Change History (9)

comment:1 Changed 2 years ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

This is documented: https://docs.djangoproject.com/en/1.5/topics/auth/customizing/#custom-users-and-the-built-in-auth-forms

UserCreationForm

Depends on the User model. Must be re-written for any custom user model.

comment:2 Changed 23 months ago by benjaoming

  • Resolution invalid deleted
  • Status changed from closed to new

Hi all! During the eurocon sprints, I was working to implement compatibility with swapped user models in django-wiki and django-cms.

I somewhat disagree with the documentation here that UserCreationForm and UserChangeForm should not be intended for custom models. Rather, I'd say that the forms should depends on the custom user model inheriting from AbstractBaseUser as with all the other forms in django.contrib.auth. This would give the documentation a much-needed simplification, anyways.

In brief, this is my opinion: A custom user model that inherits from AbstractBaseUser and does not break the contract should be able to use ALL the forms in django.contrib.auth.forms.

The current problem with contrib.auth.forms is that stuff breaks if the user model has been swapped (as noted by OP).

I would like to reopen this after working with implementing support for AUTH_USER_MODEL in a reusable app that has a couple of utility forms for account handling. They inherit from django.contrib.auth.forms and they are as optional as the forms in django.contrib.auth.forms. The reason they inherit from django.contrib.auth.forms is because of an assumption that this is the right way to implement the django.contrib.auth API, however if this breaks because people swap the user forms (which they are going to do a lot, mind you!), then there is no point at all in extending anything from django.contrib.auth.forms. Henceforth, I'd have to either stop offering account handling in the app or copy most of django.contrib.auth.forms and rewrite it to respect custom user models.

The way I would like to see this fixed is by making ALL parts of the docs say Works with any subclass of AbstractBaseUser, and will adapt to use the field defined in USERNAME_FIELD.

One of the strengths of Django is to do stuff like authentication in a uniform/extendable way. First of all, custom user models is asking people to go their own ways, but at least if they extend from existing base classes, we should gain usability. We should not make matters worse by honering the implementations that don't break the AbstractUser/AbstractBaseUser interface.

My 2 cents, thanks for a great spring.. I hope the diffs below are understandable..

django/contrib/auth/forms.py
@@ -11,11 +11,11 @@
 from django.utils.translation import ugettext, ugettext_lazy as _
 
 from django.contrib.auth import authenticate, get_user_model
-from django.contrib.auth.models import User
 from django.contrib.auth.hashers import UNUSABLE_PASSWORD, identify_hasher
 from django.contrib.auth.tokens import default_token_generator
 from django.contrib.sites.models import get_current_site
 
+User = get_user_model()
 
 UNMASKED_DIGITS_TO_SHOW = 6
 
@@ -167,8 +167,7 @@
         super(AuthenticationForm, self).__init__(*args, **kwargs)
 
         # Set the label for the "username" field.
-        UserModel = get_user_model()
-        self.username_field = UserModel._meta.get_field(UserModel.USERNAME_FIELD)
+        self.username_field = User._meta.get_field(User.USERNAME_FIELD)
         if not self.fields['username'].label:
             self.fields['username'].label = capfirst(self.username_field.verbose_name)
 
@@ -215,9 +214,8 @@
         """
         Validates that an active user exists with the given email address.
         """
-        UserModel = get_user_model()
         email = self.cleaned_data["email"]
-        self.users_cache = UserModel._default_manager.filter(email__iexact=email)
+        self.users_cache = User._default_manager.filter(email__iexact=email)
         if not len(self.users_cache):
             raise forms.ValidationError(self.error_messages['unknown'])
         if not any(user.is_active for user in self.users_cache):

Also, to avoid circularity in imports, calling get_user_model cannot happen before models are loaded (right?), so django.contrib.auth.forms cannot be loaded before models are initialized - for instance in django.contrib.admin.sites, the following issue had to be corrected:

(This is - I think - an example of https://code.djangoproject.com/ticket/19753)

django/contrib/admin/sites.py
@@ -1,7 +1,6 @@
 from functools import update_wrapper
 from django.http import Http404, HttpResponseRedirect
 from django.contrib.admin import ModelAdmin, actions
-from django.contrib.admin.forms import AdminAuthenticationForm
 from django.contrib.auth import REDIRECT_FIELD_NAME
 from django.contrib.contenttypes import views as contenttype_views
 from django.views.decorators.csrf import csrf_protect
@@ -325,6 +324,7 @@
         }
         context.update(extra_context or {})
 
+        from django.contrib.admin.forms import AdminAuthenticationForm
         defaults = {
             'extra_context': context,
             'current_app': self.name,

comment:3 Changed 22 months ago by james@…

what benjaoming said.

comment:4 Changed 22 months ago by akaariai

Hmmh, could the different forms be optional elements of the custom user model package? So, you could just reference get_user_model().user_creation_form and use that.

Now the creator of the custom user model could do:

class MyUser(AbstractBaseUser):
    ...

class MyUserAuthenticationForm(UserAuthenticationForm):
    class Meta:
        model = MyUser  # Assuming this way of modelform subclassing actually works...

MyUser.user_authentication_form = MyUserAuthenticationForm

This adds a bit of boilerplate code when using custom user models...

comment:5 Changed 22 months ago by russellm

  • Resolution set to invalid
  • Status changed from new to closed

I completely agree that this is desirable in theory. My problem is that I simply don't see how it is possible in practice.

The contract for AbstractAuthUser is that the User has a unique identifying field. We have no prior knowledge whether it is a char field, and email field, or even an integer field. If it's a char field, we don't know if it has any length or other validation constraints. And we don't know if there are any other fields on the user model that are needed for authentication purposes (for example, you may use a domain and a username as a pair for login purposes). And that's when you're only dealing with the *username* field. It doesn't deal with any of the other fields that UserCreationForm or UserChangeForm needs to deal with.

So, I'm closing this again. However, it's clearly an issue that people aren't happy with, so if you want to pursue this, please take it to Django-dev.

comment:6 Changed 15 months ago by denis_g

  • Resolution invalid deleted
  • Status changed from closed to new

Hello everyone!

I ran into this issue when subclassing UserCreationForm and I have an idea about partial solution. It doesn't solve the problem with custom field names, but since the UserCreationForm class has the Meta subclass, I think it would be appropriate to replace User with self.Meta.model.

django/contrib/auth/forms.py
@@ -95,8 +95,8 @@
         # but it sets a nicer error message than the ORM. See #13147.
         username = self.cleaned_data["username"]
         try:
-            User._default_manager.get(username=username)
-        except User.DoesNotExist:
+            self.Meta.model._default_manager.get(username=username)
+        except self.Meta.model.DoesNotExist:
             return username
         raise forms.ValidationError(
             self.error_messages['duplicate_username'],

comment:7 Changed 15 months ago by bmispelon

  • Resolution set to invalid
  • Status changed from new to closed

Hi,

There's a patch (by yours truly) for what you're suggesting in #19353.
It seems that ticket has gotten stuck somehow and I'm not too sure why.

I'll add it to my list of things to check out when I have some time (like in a week or so).

In the meantime, I'll revert this ticket back to invalid.

comment:8 Changed 12 months ago by anonymous

  • Has patch set
  • Resolution changed from invalid to fixed
  • Triage Stage changed from Unreviewed to Accepted

comment:9 Changed 12 months ago by anonymous

Solved

Note: See TracTickets for help on using tickets.
Back to Top