Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#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

Attachments (1)

tests-30776.diff (1.0 KB ) - added by Mariusz Felisiak 5 years ago.
Regression test.

Download all attachments as: .zip

Change History (12)

by Mariusz Felisiak, 5 years ago

Attachment: tests-30776.diff added

Regression test.

comment:1 by Sam Reynolds, 5 years ago

Owner: changed from nobody to Sam Reynolds
Status: newassigned

comment:2 by Sam Reynolds, 5 years ago

Cc: Sam Reynolds added
Has patch: set

comment:3 by gopackgo90, 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 Mariusz Felisiak, 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 Mariusz Felisiak, 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.

Last edited 5 years ago by Mariusz Felisiak (previous) (diff)

comment:6 by Carlton Gibson, 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...)

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

comment:7 by Carlton Gibson, 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.

Version 0, edited 5 years ago by Carlton Gibson (next)

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 6c9778a:

Fixed #30776 -- Restored max length validation on AuthenticationForm.UsernameField.

Regression in 5ceaf14686ce626404afb6a5fbd3d8286410bf13.

Thanks gopackgo90 for the report and Mariusz Felisiak for tests.

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

In e74ca022:

[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

comment:10 by Sam Reynolds, 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 Carlton Gibson, 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. 👍

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