Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#29449 closed Bug (fixed)

UserCreationForm and UserChangeForm don't work if username isn't a CharField

Reported by: Sławek Ehlert Owned by: nobody
Component: contrib.auth Version: 2.1
Severity: Release blocker Keywords: auth forms UserCreationForm UserChangeForm
Cc: markush, Carlton Gibson, Mariusz Felisiak Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In 3333d935d2914cd80cf31f4803821ad5c0e2a51d (part of #28757 ticket) UserCreationForm (and by extension also UserChangeForm) was adjusted to support custom user models without overriding the form. The problem is that the implementation there assumes that the USERNAME_FIELD has to be a subclass of CharField since it uses UsernameField in the field_classes mapping.

Now to the fun part. Django already has some tests that should fail in such case (for example see https://github.com/django/django/blob/825f0beda804e48e9197fcf3b0d909f9f548aa47/tests/auth_tests/test_management.py#L435-L491), but the failure is actually "swallowed" when running the whole test suite. When running those tests individually we can see the failure.
Tim Graham already noticed something similar in https://code.djangoproject.com/ticket/28757#comment:7.
I.e.

 ./runtests.py auth_tests

works, but

./runtests.py auth_tests.test_management.CreatesuperuserManagementCommandTestCase.test_fields_with_fk

doesn't. I'm not sure why the failure is hidden in the former case (I suspect it has to do something with the override_settings decorator and importlib.reloading done in the reload_auth_forms function from the commit I've mentioned). The stacktrace from the latter case looks like this:

ERROR: test_fields_with_fk (auth_tests.test_management.CreatesuperuserManagementCommandTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/slafs/testing/django/django/django/test/utils.py", line 364, in inner
    with self as context:
  File "/Users/slafs/testing/django/django/django/test/utils.py", line 336, in __enter__
    return self.enable()
  File "/Users/slafs/testing/django/django/django/test/utils.py", line 405, in enable
    setting=key, value=new_value, enter=True)
  File "/Users/slafs/testing/django/django/django/dispatch/dispatcher.py", line 175, in send
    for receiver in self._live_receivers(sender)
  File "/Users/slafs/testing/django/django/django/dispatch/dispatcher.py", line 175, in <listcomp>
    for receiver in self._live_receivers(sender)
  File "/Users/slafs/testing/django/django/django/test/signals.py", line 195, in user_model_swapped
    from django.contrib.auth import forms
  File "/Users/slafs/testing/django/django/django/contrib/auth/forms.py", line 63, in <module>
    class UserCreationForm(forms.ModelForm):
  File "/Users/slafs/testing/django/django/django/forms/models.py", line 256, in __new__
    apply_limit_choices_to=False,
  File "/Users/slafs/testing/django/django/django/forms/models.py", line 172, in fields_for_model
    formfield = f.formfield(**kwargs)
  File "/Users/slafs/testing/django/django/django/db/models/fields/related.py", line 956, in formfield
    **kwargs,
  File "/Users/slafs/testing/django/django/django/db/models/fields/related.py", line 418, in formfield
    return super().formfield(**defaults)
  File "/Users/slafs/testing/django/django/django/db/models/fields/__init__.py", line 890, in formfield
    return form_class(**defaults)
  File "/Users/slafs/testing/django/django/django/forms/fields.py", line 213, in __init__
    super().__init__(**kwargs)
TypeError: __init__() got an unexpected keyword argument 'limit_choices_to'

Since auth_tests.CustomUserWithFK model uses a ForeignKey as a USERNAME_FIELD, the instantiation of django.contrib.auth.forms.UsernameField is failing. However, that test is a bit implicit as I'm not entirely sure why Django imports UserCreationForm in the first place (the stacktrace makes me think it's because of the signal mechanisms).

I'd suggest the following steps to go forward with this:

1) Write a more explicit regression test in auth_tests.test_forms that's using auth_tests.CustomUserWithFK as a user model (I'll try to do that).
2) Make adjustments in auth_tests.test_forms so that the mentioned tests will actually fail even when running the whole test suite (I think Tim Graham already tried this in https://code.djangoproject.com/ticket/28757#comment:8).
3) Fix UserCreationForm so it supports different types of fields for USERNAME_FIELD.

In my humble opinion this is a release blocker (I haven't marked this ticket as such, though).

Change History (17)

comment:1 by Markus Holtermann, 7 years ago

Severity: NormalRelease blocker
Triage Stage: UnreviewedAccepted

I can confirm this.

Using something like UserModel._meta.get_field(UserModel.USERNAME_FIELD).formfield() to derive the form field for a the current username field _may_ make sense. However, this won't work with the normalization we currently do in the form's UsernameField.

comment:2 by Sławek Ehlert, 7 years ago

I've added some explicit tests in PR#10020.

Unfortunately it causes more failures than the ones I've introduced with those new tests there.

What happens is:

  • the test I've added overrides the AUTH_USER_MODEL setting with a model that has a FK as a username field (auth_tests.CustomUserWithFK).
  • the test fails on the override_settings part already, since the reload_auth_forms receiver fails to reload the django.contrib.auth.forms module with that user model in place (fails with a similar TypeError that refers to 'limit_choices_to' argument when trying to instantiate the UsernameField)[*]
  • because of the override_settings bug (see #29467) other tests still see the auth_tests.CustomUserWithFK as a current user model.

[*] - the exact traceback for the mentioned error is:

======================================================================
ERROR: test_custom_form_username_not_charfield (auth_tests.test_forms.UserChangeFormTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/Cellar/python/3.6.5/Frameworks/Python.framework/Versions/3.6/lib/python3.6/unittest/case.py", line 59, in testPartExecutor
    yield
  File "/usr/local/Cellar/python/3.6.5/Frameworks/Python.framework/Versions/3.6/lib/python3.6/unittest/case.py", line 605, in run
    testMethod()
  File "/Users/slafs/testing/django/django/django/test/utils.py", line 364, in inner
    with self as context:
  File "/Users/slafs/testing/django/django/django/test/utils.py", line 336, in __enter__
    return self.enable()
  File "/Users/slafs/testing/django/django/django/test/utils.py", line 405, in enable
    setting=key, value=new_value, enter=True)
  File "/Users/slafs/testing/django/django/django/dispatch/dispatcher.py", line 175, in send
    for receiver in self._live_receivers(sender)
  File "/Users/slafs/testing/django/django/django/dispatch/dispatcher.py", line 175, in <listcomp>
    for receiver in self._live_receivers(sender)
  File "/Users/slafs/testing/django/django/tests/auth_tests/test_forms.py", line 38, in reload_auth_forms
    reload(django.contrib.auth.forms)
  File "/Users/slafs/.virtualenvs/django/lib/python3.6/importlib/__init__.py", line 166, in reload
    _bootstrap._exec(spec, module)
  File "<frozen importlib._bootstrap>", line 618, in _exec
  File "<frozen importlib._bootstrap_external>", line 678, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/Users/slafs/testing/django/django/django/contrib/auth/forms.py", line 63, in <module>
    class UserCreationForm(forms.ModelForm):
  File "/Users/slafs/testing/django/django/django/forms/models.py", line 256, in __new__
    apply_limit_choices_to=False,
  File "/Users/slafs/testing/django/django/django/forms/models.py", line 172, in fields_for_model
    formfield = f.formfield(**kwargs)
  File "/Users/slafs/testing/django/django/django/db/models/fields/related.py", line 956, in formfield
    **kwargs,
  File "/Users/slafs/testing/django/django/django/db/models/fields/related.py", line 418, in formfield
    return super().formfield(**defaults)
  File "/Users/slafs/testing/django/django/django/db/models/fields/__init__.py", line 890, in formfield
    return form_class(**defaults)
  File "/Users/slafs/testing/django/django/django/forms/fields.py", line 213, in __init__
    super().__init__(**kwargs)
TypeError: __init__() got an unexpected keyword argument 'limit_choices_to'

comment:3 by Tim Graham, 7 years ago

I don't understand why this issue is described as a regression. Using a custom user model with UserCreationForm and UserChangeForm is a new feature, so I wouldn't describe any issues there as a regression. A regression would be something that worked in older versions of Django that no longer works. Am I misunderstanding?

in reply to:  3 comment:4 by Sławek Ehlert, 7 years ago

Replying to Tim Graham:

I don't understand why this issue is described as a regression. Using a custom user model with UserCreationForm and UserChangeForm is a new feature, so I wouldn't describe any issues there as a regression. A regression would be something that worked in older versions of Django that no longer works. Am I misunderstanding?

Nope, all good. You're correct about the naming here. My bad.

comment:5 by Tim Graham, 7 years ago

Summary: Regression in UserCreationForm (and UserChangeForm) for custom user modelsUserCreationForm and UserChangeForm don't work if username isn't a CharField

If a patch isn't forthcoming (the best way to avoid the issue that Markus pointed out in comment 1 isn't obvious to me), I think we could deescalate from a release blocker by documenting this limitation.

comment:6 by Sławek Ehlert, 7 years ago

I'd like to submit a patch for that, but currently I was focusing more on solving #29467 first.

comment:7 by Carlton Gibson, 7 years ago

Has patch: set

I've opened PR to adjust the docs here.

I think we could deescalate from a release blocker by documenting this limitation.

Hopefully that lets us move forward.

comment:8 by Sławek Ehlert, 7 years ago

Well, I think the more pressing problem is not the feature set Django supports and the docs, but the fact that the change mentioned here breaks the test suite and can break people's projects.

Also by having a auth_tests.CustomUserWithFK model in the test suite, one would think that this kind of model is supported by Django, which is not true at this point.

Moreover, having a similar user model in a project means that a developer can't even import from django.contrib.auth.forms module. And what if something imports that module that's not in developer's control? E.g. currently django/contrib/admin/forms.py, django/contrib/auth/admin.py and django/contrib/auth/views.py are importing something from that module, so that means on fresh project with admin enabled with that kind of a user model, developer can't even run ./manage.py --help.

comment:9 by Sławek Ehlert, 7 years ago

So I think, regarding this feature - we should either fix it or ditch it completely.

I made some substantial progress on #29467 lately. I'd like to try and tackle this ticket next (although I have some time constraints and the progress may be slow on this).

comment:10 by Carlton Gibson, 7 years ago

Cc: Carlton Gibson added

...we should either fix it or ditch it completely.

Yes. OK.

Last edited 7 years ago by Carlton Gibson (previous) (diff)

comment:11 by Carlton Gibson, 7 years ago

Has patch: unset

comment:13 by Mariusz Felisiak, 7 years ago

Cc: Mariusz Felisiak added

comment:14 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: newclosed

In f3fa86a:

Fixed #29449 -- Reverted "Fixed #28757 -- Allowed using contrib.auth forms without installing contrib.auth."

This reverts commit 3333d935d2914cd80cf31f4803821ad5c0e2a51d due to
a crash if USERNAME_FIELD isn't a CharField.

comment:15 by Tim Graham <timograham@…>, 6 years ago

In 78f502c:

[2.1.x] Fixed #29449 -- Reverted "Fixed #28757 -- Allowed using contrib.auth forms without installing contrib.auth."

This reverts commit 3333d935d2914cd80cf31f4803821ad5c0e2a51d due to
a crash if USERNAME_FIELD isn't a CharField.

Backport of f3fa86a89b3b85242f49b2b9acf58b5ea35acc1f from master

comment:16 by Tim Graham <timograham@…>, 6 years ago

In d3449fa:

Refs #29449 -- Removed release note for "Allowed using contrib.auth forms without installing contrib.auth."

The code was reverted in f3fa86a89b3b85242f49b2b9acf58b5ea35acc1f.

comment:17 by Tim Graham <timograham@…>, 6 years ago

In c903e905:

[2.1.x] Refs #29449 -- Removed release note for "Allowed using contrib.auth forms without installing contrib.auth."

The code was reverted in f3fa86a89b3b85242f49b2b9acf58b5ea35acc1f.
Backport of d3449faaa915a08c275b35de01e66a7ef6bdb2dc from master

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