[Django] #30776: AuthenticationForm's username field doesn't set maxlength HTML attribute.
Django
----------------------------------------+------------------------
Reporter: felixxm | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: master
Severity: Normal | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
----------------------------------------+------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/30776>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: felixxm | Owner: nobody
Type: Bug | Status: new
Component: contrib.auth | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* Attachment "tests-30776.diff" added.
Regression test.
Django
Reporter: felixxm | Owner: smjreynolds
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by smjreynolds):
* owner: nobody => smjreynolds
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/30776#comment:1>
Django
Reporter: felixxm | Owner: Sam Reynolds
Type: Bug | Status: assigned
Component: contrib.auth | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sam Reynolds):
* cc: Sam Reynolds (added)
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/11790 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/30776#comment:2>
Django
Reporter: felixxm | Owner: Sam Reynolds
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by gopackgo90):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30776#comment:3>
Django
Reporter: felixxm | Owner: Sam Reynolds
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/30776#comment:4>
Django
Reporter: felixxm | Owner: Sam Reynolds
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
@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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30776#comment:5>
Django
Reporter: felixxm | Owner: Sam Reynolds
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
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...)
--
Ticket URL: <https://code.djangoproject.com/ticket/30776#comment:6>
Django
Reporter: felixxm | Owner: Sam Reynolds
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/30776#comment:7>
Django
Reporter: felixxm | Owner: Sam Reynolds
Component: contrib.auth | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"6c9778a58e4f680db180d4cc9dc5639d2ec1b40c" 6c9778a]:
{{{
#!CommitTicketReference repository=""
revision="6c9778a58e4f680db180d4cc9dc5639d2ec1b40c"
Fixed #30776 -- Restored max length validation on
AuthenticationForm.UsernameField.
Regression in 5ceaf14686ce626404afb6a5fbd3d8286410bf13.
Thanks gopackgo90 for the report and Mariusz Felisiak for tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30776#comment:8>
Django
Reporter: felixxm | Owner: Sam Reynolds
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"e74ca0226a0699ac046d59159e46fddf52ff93b5" e74ca022]:
{{{
#!CommitTicketReference repository=""
revision="e74ca0226a0699ac046d59159e46fddf52ff93b5"
[3.0.x] Fixed #30776 -- Restored max length validation on
AuthenticationForm.UsernameField.
Regression in 5ceaf14686ce626404afb6a5fbd3d8286410bf13.
Thanks gopackgo90 for the report and Mariusz Felisiak for tests.
Backport of 6c9778a58e4f680db180d4cc9dc5639d2ec1b40c from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/30776#comment:9>
Django
Reporter: felixxm | Owner: Sam Reynolds
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sam Reynolds):
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/30776#comment:10>
Django
Reporter: felixxm | Owner: Sam Reynolds
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Carlton Gibson):
No problem Sam! Thank you for your efforts. If nothing else, going through
it refreshed my ''Swapable Models: Here be Dragons'' chops. 👍
--
Ticket URL: <https://code.djangoproject.com/ticket/30776#comment:11>