Opened 21 months ago

Closed 21 months ago

Last modified 21 months ago

#34438 closed Bug (fixed)

UserCreationForm.clean_username() crashes with a custom user model.

Reported by: Gary Jarrel Owned by: Gary Jarrel
Component: contrib.auth Version: 4.2
Severity: Release blocker Keywords:
Cc: gary@…, Paul Schilling Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Our project users a custom user model defined in settings via AUTH_USER_MODEL

There is a basic test case:

def test_username_validation_error_msg(self, user: User):
        # The user already exists,
        # hence cannot be created.
        form = UserAdminCreationForm(
            {
                "username": user.username,
                "password1": user.password,
                "password2": user.password,
            }
        )

        assert not form.is_valid()
        assert len(form.errors) == 1
        assert "username" in form.errors
        assert form.errors["username"][0] == _("This username has already been taken.")

This works in Django 4.1, however when I ran the test suite against 4.2rc1 and the current main branch, I get failures.

AttributeError: Manager isn't available; 'auth.User' has been swapped for 'users.User'

with the trace

../../contrib/django/django/forms/forms.py:197: in is_valid
    return self.is_bound and not self.errors
../../contrib/django/django/forms/forms.py:192: in errors
    self.full_clean()
../../contrib/django/django/forms/forms.py:327: in full_clean
    self._clean_fields()
../../contrib/django/django/forms/forms.py:342: in _clean_fields
    value = getattr(self, "clean_%s" % name)()
../../contrib/django/django/contrib/auth/forms.py:158: in clean_username
    if username and User.objects.filter(username__iexact=username).exists():

I believe it relates to this commit

The line

if username and User.objects.filter(username__iexact=username).exists()

When I changed the line to:

if username and UserModel.objects.filter(username__iexact=username).exists()

The error was resolved.

This could potentially be related to: 28608, but it seems like a very old ticket, when compared to the commit which was done in December 22

As a side note, the test then failed due to the change in the error message from:

"This username has already been taken."

to

A user with that username already exists.

But that's not a big issue.

Change History (17)

comment:1 by Gary Jarrel, 21 months ago

Summary: In Django 4.2 UserCreationForm fails with Manager isn't available; 'auth.User' has been swapped for 'users.User'In Django 4.2 UserCreationForm fails with Manager isn't available; 'auth.User' has been swapped for <custom user model>

in reply to:  description comment:2 by Mariusz Felisiak, 21 months ago

Cc: Paul Schilling added
Severity: NormalRelease blocker
Summary: In Django 4.2 UserCreationForm fails with Manager isn't available; 'auth.User' has been swapped for <custom user model>UserCreationForm.clean_username() crashes with a custom user model.
Triage Stage: UnreviewedAccepted

This works in Django 4.1, however when I ran the test suite against 4.2rc1 and the current main branch, I get failures.

It's documented that the UserChangeForm is tied to User and "need to be rewritten or extended to work with a custom user model" (you can find a full example in docs). However, I agree that could use self._meta.model instead of User to make it easier and not to break some existing configurations:

  • django/contrib/auth/forms.py

    diff --git a/django/contrib/auth/forms.py b/django/contrib/auth/forms.py
    index 0376d17709..a1650a7ce2 100644
    a b class UserCreationForm(BaseUserCreationForm):  
    155155    def clean_username(self):
    156156        """Reject usernames that differ only in case."""
    157157        username = self.cleaned_data.get("username")
    158         if username and User.objects.filter(username__iexact=username).exists():
     158        if username and self._meta.model.objects.filter(username__iexact=username).exists():
    159159            raise forms.ValidationError(self.error_messages["unique"], code="unique")
    160160        else:
    161161            return username

Does it work for you?

comment:3 by Gary Jarrel, 21 months ago

Yes the change to:

self._meta.model.objects.filter

does indeed work and the form validations tests pass.

comment:4 by Gary Jarrel, 21 months ago

Out of interest if I may ask, I just tried to write a test to reproduce the behaviour, then a patch, in a forked Django Repo, for example if I wanted to contribute a fix back to the code base via a pull request.

My first attempt was to simply add another test to the UserCreationFormTest, by copying the existing test_case_insensitive_username and adding {{@override_settings}} decorator as follows:

@override_settings(AUTH_USER_MODEL="auth_tests.CustomUser")
def test_case_insensitive_username_with_external_user_model(self):
    data = {
        "username": "TeStClIeNt",
        "password1": "test123",
        "password2": "test123",
    }
    form = UserCreationForm(data)
    self.assertFalse(form.is_valid())
    self.assertEqual(
        form["username"].errors,
        ["A user with that username already exists."],
    )

So I did that, and interestingly enough I started getting the same error as I did in our project:

ERROR: test_case_insensitive_username_with_external_user_model (auth_tests.test_forms.UserCreationFormTest.test_case_insensitive_username_with_external_user_model)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/garyj/.pyenv/versions/3.11.2/lib/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/home/garyj/.pyenv/versions/3.11.2/lib/python3.11/unittest/case.py", line 623, in run
    self._callTestMethod(testMethod)
    ^^^^^^^^^^^^^^^^^
  File "/home/garyj/.pyenv/versions/3.11.2/lib/python3.11/unittest/case.py", line 579, in _callTestMethod
    if method() is not None:
    ^^^^^^^^^^^^^^^^^
  File "/home/garyj/dev/contrib/django/django/test/utils.py", line 443, in inner
    return func(*args, **kwargs)
    ^^^^^^^^^^^^^^^^^
  File "/home/garyj/dev/contrib/django/tests/auth_tests/test_forms.py", line 383, in test_case_insensitive_username_with_external_user_model
    self.assertFalse(form.is_valid())
    ^^^^^^^^^^^^^^^^^
  File "/home/garyj/dev/contrib/django/django/forms/forms.py", line 197, in is_valid
    return self.is_bound and not self.errors
    ^^^^^^^^^^^^^^^^^
  File "/home/garyj/dev/contrib/django/django/forms/forms.py", line 192, in errors
    self.full_clean()
  File "/home/garyj/dev/contrib/django/django/forms/forms.py", line 327, in full_clean
    self._clean_fields()
    ^^^^^^^^^^^^^^^^^
  File "/home/garyj/dev/contrib/django/django/forms/forms.py", line 342, in _clean_fields
    value = getattr(self, "clean_%s" % name)()
    ^^^^^^^^^^^^^^^^^
  File "/home/garyj/dev/contrib/django/django/contrib/auth/forms.py", line 160, in clean_username
    and self._meta.model.objects.filter(username__iexact=username).exists()
    ^^^^^^^^^^^^^^^^^
  File "/home/garyj/dev/contrib/django/django/db/models/manager.py", line 196, in __get__
    raise AttributeError(
    ^^^^^^^^^^^^^^^^^
AttributeError: Manager isn't available; 'auth.User' has been swapped for 'auth_tests.CustomUser'

However on the same token the change self._meta.model.objects.filter(username__iexact=username).exists() is working in our project.

So I am just wondering what I am missing or doing wrong here?

Thank you

Gary

in reply to:  4 comment:5 by Mariusz Felisiak, 21 months ago

Owner: changed from nobody to Gary Jarrel
Status: newassigned

Replying to Gary Jarrel:

Out of interest if I may ask, I just tried to write a test to reproduce the behaviour, then a patch, in a forked Django Repo, for example if I wanted to contribute a fix back to the code base via a pull request.

Please do!

However on the same token the change self._meta.model.objects.filter(username__iexact=username).exists() is working in our project.

So I am just wondering what I am missing or doing wrong here?

You must still subclass UserCreationForm to work with a custom user model, e.g.

{{#!diff
diff --git a/tests/auth_tests/test_forms.py b/tests/auth_tests/test_forms.py
index c3ce1f570f..16b39a08d7 100644
--- a/tests/auth_tests/test_forms.py
+++ b/tests/auth_tests/test_forms.py
@@ -372,6 +372,32 @@ class UserCreationFormTest(TestDataMixin, TestCase):

A user with that username already exists.,

)


+ @override_settings(AUTH_USER_MODEL="auth_tests.ExtensionUser")
+ def test_case_insensitive_username_custom_user(self):
+ class CustomUserCreationForm(UserCreationForm):
+ class Meta(UserCreationForm.Meta):
+ model = ExtensionUser
+ fields = UserCreationForm.Meta.fields + ("date_of_birth",)
+
+ ExtensionUser.objects.create_user(
+ username="testclient",
+ password="password",
+ email="testclient@…",
+ date_of_birth=datetime.date(1986, 3, 4),
+ )
+ data = {
+ "username": "TeStClIeNt",
+ "password1": "test123",
+ "password2": "test123",
+ "date_of_birth": "1988-02-24",
+ }
+ form = CustomUserCreationForm(data)
+ self.assertIs(form.is_valid(), False)
+ self.assertEqual(
+ formusername.errors,
+ A user with that username already exists.,
+ )
+

# To verify that the login form rejects inactive users, use an authentication
# backend that allows them.

}}}

Version 0, edited 21 months ago by Mariusz Felisiak (next)

comment:6 by Gary Jarrel, 21 months ago

Of course that makes sense, as we use a custom form in the project as well.

I have made the code change (well copied yours) will push the changes up to my forked repo and post back here.

GitHub seems to be having some outages and it's refusing the pushes and new forks.

comment:7 by Gary Jarrel, 21 months ago

I've also mentioned in my initial ticket a side note about the error message when a unique constraint is violated for a username.

I then noticed that our UserAdminCreationForm actually defines a custom error message:

class UserAdminCreationForm(admin_forms.UserCreationForm):
    class Meta(admin_forms.UserCreationForm.Meta):
        model = User
        error_messages = {"username": {"unique": _("This username has already been taken.")}}

I than ran tests again. With Django 4.1 the test line: assert form.errors["username"][0] == _("This username has already been taken.") passes.

However with 4.2rc1 and current main branch the test fails. Seems like the error messages are not taken into account.

So in my Django Fork, I have added another test in tests.auth_tests.test_forms.UserCreationFormTest as follows:

@override_settings(AUTH_USER_MODEL="auth_tests.ExtensionUser")
def test_case_insensitive_username_custom_user_override_error_message(self):
    class CustomUserCreationForm(UserCreationForm):
        class Meta(UserCreationForm.Meta):
            model = ExtensionUser
            fields = UserCreationForm.Meta.fields + ("date_of_birth",)
            
            # CUSTOM ERROR MESSAGE
            error_messages = {"username": {"unique": _("Username gone")}}

    ExtensionUser.objects.create_user(
        username="anotherclient",
        password="password",
        email="anotehrclient@example.com",
        date_of_birth=datetime.date(1980, 1, 1),
    )

    data = {
        "username": "AnotherClienT",
        "password1": "test123",
        "password2": "test123",
        "date_of_birth": "1980-01-01",
    }

    form = CustomUserCreationForm(data)
    self.assertIs(form.is_valid(), False)
    self.assertEqual(
        form["username"].errors,
        ["Username gone"],
    )

    assert form["username"].errors[0] == _(
        "Username gone"
    )
    assert form.errors["username"][0] == _(
        "Username gone"
    )

The test has also failed. Error Traceback:

FAIL: test_case_insensitive_username_custom_user_override_error_message (auth_tests.test_forms.UserCreationFormTest.test_case_insensitive_username_custom_user_override_error_message)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/garyj/.pyenv/versions/3.11.2/lib/python3.11/unittest/case.py", line 57, in testPartExecutor
    yield
  File "/home/garyj/.pyenv/versions/3.11.2/lib/python3.11/unittest/case.py", line 623, in run
    self._callTestMethod(testMethod)
    ^^^^^^^^^^^^^^^^^
  File "/home/garyj/.pyenv/versions/3.11.2/lib/python3.11/unittest/case.py", line 579, in _callTestMethod
    if method() is not None:
    ^^^^^^^^^^^^^^^^^
  File "/home/garyj/dev/contrib/django/django/test/utils.py", line 443, in inner
    return func(*args, **kwargs)
    ^^^^^^^^^^^^^^^^^
  File "/home/garyj/dev/contrib/django/tests/auth_tests/test_forms.py", line 428, in test_case_insensitive_username_custom_user_override_error_message
    self.assertEqual(
  File "/home/garyj/.pyenv/versions/3.11.2/lib/python3.11/unittest/case.py", line 873, in assertEqual
    assertion_func(first, second, msg=msg)
    ^^^^^^^^^^^^^^^^^
  File "/home/garyj/.pyenv/versions/3.11.2/lib/python3.11/unittest/case.py", line 866, in _baseAssertEqual
    raise self.failureException(msg)
    ^^^^^^^^^^^^^^^^^
AssertionError: ['A user with that username already exists.'] != ['Username gone']

Seems to me like this is an issue if it works in 4.1 but not 4.2rc1, nor current main branch, or am I doing something wrong?

comment:8 by Gary Jarrel, 21 months ago

I have created a branch with the fixes and tests for the main issue that was raised with the custom user models in my repo here:

https://github.com/oizik/django/tree/ticket_34438

comment:9 by Sarah Boyce, 21 months ago

I think you might be defining error_messages in the wrong place, try this?

    class CustomUserCreationForm(UserCreationForm):
        error_messages = {
            **UserCreationForm.error_messages,
            "username": {"unique": _("Username gone")}
        }

        class Meta(UserCreationForm.Meta):
            model = ExtensionUser
            fields = UserCreationForm.Meta.fields + ("date_of_birth",)

Related to this, I found the docs slightly inconsistent.
According to the newly documented BaseUserCreationForm https://docs.djangoproject.com/en/4.2/topics/auth/default/#django.contrib.auth.forms.BaseUserCreationForm, that new form is the recommended base class if you need to customise the form.
But that is now slightly inconsistent to the code snippet here: https://docs.djangoproject.com/en/4.2/topics/auth/customizing/#custom-users-and-the-built-in-auth-forms which is still inheriting from UserCreationForm maybe it makes sense to update this also?

Last edited 21 months ago by Sarah Boyce (previous) (diff)

comment:10 by Gary Jarrel, 21 months ago

Thank you for pointing out the changes in the docs, I did not see this: https://docs.djangoproject.com/en/4.2/topics/auth/default/#django.contrib.auth.forms.BaseUserCreationForm

I have just made the changes to the test as per the suggestion, however the test still fails.

Understandably it's a pretty obscure case, i.e. adding a custom admin form, and changing the error message. So I am not sure how to handle it, should it be documented somewhere about these error messages, or is it a bigger problem somewhere with the way error messages are handled?

Originally our project was started from the Django Cookiecutter, which had the error_messages set up as seen in forms.py. This makes me think that anyone who is using this cookie cutter will start getting errors in tests in Django 4.2 onward.

comment:11 by Gary Jarrel, 21 months ago

Has patch: set

comment:12 by Mariusz Felisiak, 21 months ago

Patch needs improvement: set

Gary, Agreed, we should reuse the unique error message defined for fields and respect the overriding error messages in Meta (see comment).

comment:13 by Gary Jarrel, 21 months ago

I've added the code as per the comment, and have update the pull request following the advice in Rebasing branches. I hope I am following the right procedures here for contributing (it is my first time with Django).

Also ran the tests in our project against the updated code base and everything is passing :)

comment:14 by Gary Jarrel, 21 months ago

Patch needs improvement: unset

comment:15 by Mariusz Felisiak, 21 months ago

Triage Stage: AcceptedReady for checkin

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 21 months ago

Resolution: fixed
Status: assignedclosed

In 99ba5b43:

[4.2.x] Fixed #34438 -- Reallowed extending UserCreationForm.

Regression in 298d02a77a69321af8c0023df3250663e9d1362d.

Backport of fcc7dc5781667932bf0bf8bec76df458836e5e95 from main

comment:17 by Mariusz Felisiak <felisiak.mariusz@…>, 21 months ago

In fcc7dc5:

Fixed #34438 -- Reallowed extending UserCreationForm.

Regression in 298d02a77a69321af8c0023df3250663e9d1362d.

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