#30776 closed Bug (fixed)
AuthenticationForm's username field doesn't set maxlength HTML attribute.
Reported by: | Mariusz Felisiak | Owned by: | Sam Reynolds |
---|---|---|---|
Component: | contrib.auth | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Sam Reynolds | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
AuthenticationForm's username field doesn't render with maxlength
HTML attribute anymore.
Regression introduced in #27515 and 5ceaf14686ce626404afb6a5fbd3d8286410bf13.
https://groups.google.com/forum/?utm_source=digest&utm_medium=email#!topic/django-developers/qnfSqro0DlA
https://forum.djangoproject.com/t/possible-authenticationform-max-length-regression-in-django-2-1/241
Attachments (1)
Change History (12)
by , 5 years ago
Attachment: | tests-30776.diff added |
---|
comment:1 by , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 5 years ago
Is it a good idea to change a bunch of the field properties in the form's init? Wouldn't it make more sense to set these as part of the UsernameField's init like https://github.com/gopackgo90/django/commit/dd9be672ec079e90b8dfa4a8e91f31ec8185438f? In Django 2.0, the username field also had a MaxLengthValidator when it was created, which wouldn't exist if the proposed PR is accepted.
comment:4 by , 5 years ago
Good point about MaxLengthValidator
we should restore it. I don't think that changing UsernameField.__init__()
is a good idea because it is not necessary for other forms which inherit from forms.ModelForm
, also UsernameField
can be used without max_length
.
comment:5 by , 5 years ago
@gopackgo90 After reconsideration I don't think that MaxLengthValidator
is really important here since a browser truncates username
when we set maxlength
HTML attribute. This is not a ModelForm
so we don't risk IntegrityError
etc. even if someone send manipulated POST data.
Nevermind, let's bring it back, it doesn't hurt.
comment:6 by , 5 years ago
OK 2¢ here... :)
Given this:
class UsernameField(forms.CharField)
... I think I prefer setting max_length
in UsernameField.__init__()
, if not passed as a kwarg, and so leveraging CharField
's auto-adding the validator.
(Validating the POST data seems the best chance to error early, even if we're trying an INSERT...)
Update: Bit too quick there. This issue only affects AuthenticationForm
so it looks like we must pass the kwarg, or adjust in the form __init__()
method, otherwise we're making a logic change to UsernameField
. (Such may or may not be worth doing, but not on this ticket...)
comment:7 by , 5 years ago
Is it a good idea to change a bunch of the field properties in the form's init?
Grrr. This is horrible... :) — It seems necessary, at least for the tests, because UserModel
is swappable, so when AuthenticationForm
(and others) are declared, UserModel
is auth.User
, so we can't use UserModel
to pass max_length
in the class definition.
Nor, though can we add the user name field in the form __init__()
, in order to avoid this kind of logic, and duplicating CharField.__init__()
:
- Other logic, e.g. correct label setting assumes the current setup, and...
- e.g. We swap out for IntegerFields, depending on the type of the username field.
And then, given that last, we'd need to type check before manually re-adding the validator, at which point I come back to Mariusz' suggestion that it's probably not worth it.
Sorry for the back and forth. This is significantly more complex than it at first appears.
comment:10 by , 5 years ago
Sorry Carlton. Had I been paying attention to the ticket, I might have saved you some pain. I saw the issue with swapping out the field/widget when making the change.
My rationale for sticking with the simpler option was that we can rely on the authentication process to ensure the max length of a username. After all, if the entered username is longer than the max_length
of the user model, it can't be a validusername!
comment:11 by , 5 years ago
No problem Sam! Thank you for your efforts. If nothing else, going through it refreshed my Swapable Models: Here be Dragons chops. 👍
Regression test.