Code

Opened 6 years ago

Closed 21 months ago

#7833 closed Bug (fixed)

UserCreationForm doesn't handle the case where password1 is not set

Reported by: jvloothuis Owned by: Claude Paroz <claude@…>
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)

usercreationform.patch (1.5 KB) - added by jvloothuis 6 years ago.
7833-take-2.diff (1000 bytes) - added by mir 6 years ago.

Download all attachments as: .zip

Change History (13)

Changed 6 years ago by jvloothuis

comment:1 Changed 6 years ago by Mnewman

  • milestone set to 1.0
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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.

comment:2 Changed 6 years ago by mir

The original patch is a bit twisted. Let's simply use get() and all's well.

Changed 6 years ago by mir

comment:3 Changed 6 years ago by mir

  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to 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 Changed 6 years ago by Mnewman

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to 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 Changed 6 years ago by jacob

  • Resolution set to fixed
  • Status changed from new to closed

(In [8542]) Fixed #7833: the user creation form now works when password1 isn't set.

comment:6 Changed 4 years ago by mitar

  • Cc mmitar@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version changed from SVN to 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 Changed 4 years ago by anonymous

  • milestone 1.0 deleted

comment:8 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to Bug

comment:9 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:11 Changed 21 months ago by Claude Paroz <claude@…>

  • Owner set to Claude Paroz <claude@…>
  • Resolution set to fixed
  • Status changed from reopened to closed

In [09a719a4e6820d462fc796606cb242583ad4e79d]:

Fixed #7833 -- Improved UserCreationForm password validation

Make UserCreationForm password validation similar to
SetPasswordForm and AdminPasswordChangeForm, so as the match
check is only done when both passwords are supplied.
Thanks Mitar for the suggestion.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.