Opened 16 years ago
Closed 12 years ago
#7833 closed Bug (fixed)
UserCreationForm doesn't handle the case where password1 is not set
Reported by: | Jeroen Vloothuis | Owned by: | |
---|---|---|---|
Component: | contrib.auth | Version: | 1.2 |
Severity: | Normal | Keywords: | |
Cc: | mmitar@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
The new UserCreationForm does not check wheter password1 is available in cleaned_data when checking it against the second password. It can be empty when the form is submitted and the user leaves this field blank. The patch has a test and a fix for this issue.
Attachments (2)
Change History (13)
by , 16 years ago
Attachment: | usercreationform.patch added |
---|
comment:1 by , 16 years ago
milestone: | → 1.0 |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 16 years ago
The original patch is a bit twisted. Let's simply use get() and all's well.
by , 16 years ago
Attachment: | 7833-take-2.diff added |
---|
comment:3 by , 16 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
Triage Stage: | Accepted → Ready for checkin |
For some unknown reason, trac hides a part of my patch (the one with the tests). You've got to download it for a proper view.
comment:4 by , 16 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
I still don't think this patch is ready for check in. The test still isn't a proper case (see above). I also think that the checking of the passwords should be done in clean. This section was ported from old forms and we shouldn't be afraid of moving this to the right place now.
comment:5 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:6 by , 14 years ago
Cc: | added |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
Version: | SVN → 1.2 |
I think clean_password2
is still broken in UserCreationForm
. Currently if there is an error in password1
then there is no value for it in cleaned_data
so current code adds additional warning that passwords mismatch. Which is not necessary true, they could match but just password1
failed validation.
SetPasswordForm
and AdminPasswordChangeForm
have much better clean_password2
method without this problem and I would suggest that it is used also in UserCreationForm
:
def clean_password2(self): password1 = self.cleaned_data.get('password1') password2 = self.cleaned_data.get('password2') if password1 and password2: if password1 != password2: raise forms.ValidationError(_("The two password fields didn't match.")) return password2
comment:7 by , 14 years ago
milestone: | 1.0 |
---|
comment:8 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:11 by , 12 years ago
Owner: | set to |
---|---|
Resolution: | → fixed |
Status: | reopened → closed |
This is clearly a problem. One thing about the patch in the test the data passed through to the form would include
password1 : ''
, because the field is still on the page, there just isn't anything in it.The other thing I would like to bring up is whether we would like the password checking in clean_password2 or if we should have something that more resembles django-registration, which has it in the clean method and doesn't have this problem.