diff --git a/ietf/ietfauth/forms.py b/ietf/ietfauth/forms.py index 53bf7b5888c..41828f2bf66 100644 --- a/ietf/ietfauth/forms.py +++ b/ietf/ietfauth/forms.py @@ -33,21 +33,56 @@ def clean_email(self): return email +class PasswordStrengthField(forms.CharField): + widget = PasswordStrengthInput( + attrs={ + "class": "password_strength", + "data-disable-strength-enforcement": "", # usually removed in init + } + ) + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + for pwval in password_validation.get_default_password_validators(): + if isinstance(pwval, password_validation.MinimumLengthValidator): + self.widget.attrs["minlength"] = pwval.min_length + elif isinstance(pwval, StrongPasswordValidator): + self.widget.attrs.pop( + "data-disable-strength-enforcement", None + ) + + + class PasswordForm(forms.Form): - password = forms.CharField(widget=PasswordStrengthInput(attrs={'class':'password_strength'})) + password = PasswordStrengthField() password_confirmation = forms.CharField(widget=PasswordConfirmationInput( confirm_with='password', attrs={'class':'password_confirmation'}), help_text="Enter the same password as above, for verification.",) - + + def __init__(self, *args, user=None, **kwargs): + # user is a kw-only argument to avoid interfering with the signature + # when this class is mixed with ModelForm in PersonPasswordForm + self.user = user + super().__init__(*args, **kwargs) def clean_password_confirmation(self): - password = self.cleaned_data.get("password", "") - password_confirmation = self.cleaned_data["password_confirmation"] + # clean fields here rather than a clean() method so validation is + # still enforced in PersonPasswordForm without having to override its + # clean() method + password = self.cleaned_data.get("password") + password_confirmation = self.cleaned_data.get("password_confirmation") if password != password_confirmation: - raise forms.ValidationError("The two password fields didn't match.") + raise ValidationError( + "The password confirmation is different than the new password" + ) + try: + password_validation.validate_password(password_confirmation, self.user) + except ValidationError as err: + self.add_error("password", err) return password_confirmation + def ascii_cleaner(supposedly_ascii): outside_printable_ascii_pattern = r'[^\x20-\x7F]' if re.search(outside_printable_ascii_pattern, supposedly_ascii): @@ -174,35 +209,13 @@ class Meta: exclude = ['by', 'time' ] -class ChangePasswordForm(forms.Form): +class ChangePasswordForm(PasswordForm): current_password = forms.CharField(widget=forms.PasswordInput) + field_order = ["current_password", "password", "password_confirmation"] - new_password = forms.CharField( - widget=PasswordStrengthInput( - attrs={ - "class": "password_strength", - "data-disable-strength-enforcement": "", # usually removed in init - } - ), - ) - new_password_confirmation = forms.CharField( - widget=PasswordConfirmationInput( - confirm_with="new_password", attrs={"class": "password_confirmation"} - ) - ) - - def __init__(self, user, data=None): - self.user = user - super().__init__(data) - # Check whether we have validators to enforce - new_password_field = self.fields["new_password"] - for pwval in password_validation.get_default_password_validators(): - if isinstance(pwval, password_validation.MinimumLengthValidator): - new_password_field.widget.attrs["minlength"] = pwval.min_length - elif isinstance(pwval, StrongPasswordValidator): - new_password_field.widget.attrs.pop( - "data-disable-strength-enforcement", None - ) + def __init__(self, user, *args, **kwargs): + # user arg is optional in superclass, but required for this form + super().__init__(*args, user=user, **kwargs) def clean_current_password(self): # n.b., password = None is handled by check_password and results in a failed check @@ -211,15 +224,6 @@ def clean_current_password(self): raise ValidationError("Invalid password") return password - def clean(self): - new_password = self.cleaned_data.get("new_password", "") - conf_password = self.cleaned_data.get("new_password_confirmation", "") - if new_password != conf_password: - raise ValidationError( - "The password confirmation is different than the new password" - ) - password_validation.validate_password(conf_password, self.user) - class ChangeUsernameForm(forms.Form): username = forms.ChoiceField(choices=[('-','--------')]) diff --git a/ietf/ietfauth/tests.py b/ietf/ietfauth/tests.py index dd23277b63c..a77e5bd5d58 100644 --- a/ietf/ietfauth/tests.py +++ b/ietf/ietfauth/tests.py @@ -168,18 +168,40 @@ def register_and_verify(self, email): self.assertEqual(r.status_code, 200) # password mismatch - r = self.client.post(confirm_url, { 'password': 'secret', 'password_confirmation': 'nosecret' }) + r = self.client.post( + confirm_url, { + "password": "secret-and-secure", + "password_confirmation": "not-secret-or-secure", + } + ) + self.assertEqual(r.status_code, 200) + self.assertEqual(User.objects.filter(username=email).count(), 0) + + # weak password + r = self.client.post( + confirm_url, { + "password": "password1234", + "password_confirmation": "password1234", + } + ) self.assertEqual(r.status_code, 200) self.assertEqual(User.objects.filter(username=email).count(), 0) # confirm - r = self.client.post(confirm_url, { 'name': 'User Name', 'ascii': 'User Name', 'password': 'secret', 'password_confirmation': 'secret' }) + r = self.client.post( + confirm_url, + { + "name": "User Name", + "ascii": "User Name", + "password": "secret-and-secure", + "password_confirmation": "secret-and-secure", + }, + ) self.assertEqual(r.status_code, 200) self.assertEqual(User.objects.filter(username=email).count(), 1) self.assertEqual(Person.objects.filter(user__username=email).count(), 1) self.assertEqual(Email.objects.filter(person__user__username=email).count(), 1) - # This also tests new account creation. def test_create_existing_account(self): # create account once @@ -393,6 +415,7 @@ def test_nomcom_dressing_on_profile(self): self.assertTrue(q('#volunteered')) def test_reset_password(self): + WEAK_PASSWORD="password1234" VALID_PASSWORD = "complex-and-long-valid-password" ANOTHER_VALID_PASSWORD = "very-complicated-and-lengthy-password" url = urlreverse("ietf.ietfauth.views.password_reset") @@ -450,6 +473,18 @@ def test_reset_password(self): q = PyQuery(r.content) self.assertTrue(len(q("form .is-invalid")) > 0) + # weak password + r = self.client.post( + confirm_url, + { + "password": WEAK_PASSWORD, + "password_confirmation": WEAK_PASSWORD, + }, + ) + self.assertEqual(r.status_code, 200) + q = PyQuery(r.content) + self.assertTrue(len(q("form .is-invalid")) > 0) + # confirm r = self.client.post( confirm_url, @@ -636,8 +671,8 @@ def test_change_password(self): chpw_url, { "current_password": "fiddlesticks", - "new_password": ANOTHER_VALID_PASSWORD, - "new_password_confirmation": ANOTHER_VALID_PASSWORD, + "password": ANOTHER_VALID_PASSWORD, + "password_confirmation": ANOTHER_VALID_PASSWORD, }, ) self.assertEqual(r.status_code, 200) @@ -648,14 +683,14 @@ def test_change_password(self): chpw_url, { "current_password": VALID_PASSWORD, - "new_password": ANOTHER_VALID_PASSWORD, - "new_password_confirmation": ANOTHER_VALID_PASSWORD[::-1], + "password": ANOTHER_VALID_PASSWORD, + "password_confirmation": ANOTHER_VALID_PASSWORD[::-1], }, ) self.assertEqual(r.status_code, 200) self.assertFormError( r.context["form"], - None, + "password_confirmation", "The password confirmation is different than the new password", ) @@ -664,14 +699,14 @@ def test_change_password(self): chpw_url, { "current_password": VALID_PASSWORD, - "new_password": "sh0rtpw0rd", - "new_password_confirmation": "sh0rtpw0rd", + "password": "sh0rtpw0rd", + "password_confirmation": "sh0rtpw0rd", } ) self.assertEqual(r.status_code, 200) self.assertFormError( r.context["form"], - None, + "password", "This password is too short. It must contain at least " f"{settings.PASSWORD_POLICY_MIN_LENGTH} characters." ) @@ -681,14 +716,14 @@ def test_change_password(self): chpw_url, { "current_password": VALID_PASSWORD, - "new_password": "passwordpassword", - "new_password_confirmation": "passwordpassword", + "password": "passwordpassword", + "password_confirmation": "passwordpassword", } ) self.assertEqual(r.status_code, 200) self.assertFormError( r.context["form"], - None, + "password", "This password does not meet complexity requirements " "and is easily guessable." ) @@ -698,8 +733,8 @@ def test_change_password(self): chpw_url, { "current_password": VALID_PASSWORD, - "new_password": ANOTHER_VALID_PASSWORD, - "new_password_confirmation": ANOTHER_VALID_PASSWORD, + "password": ANOTHER_VALID_PASSWORD, + "password_confirmation": ANOTHER_VALID_PASSWORD, }, ) self.assertRedirects(r, prof_url) diff --git a/ietf/ietfauth/views.py b/ietf/ietfauth/views.py index 84d54908736..4219747e122 100644 --- a/ietf/ietfauth/views.py +++ b/ietf/ietfauth/views.py @@ -529,7 +529,7 @@ def confirm_password_reset(request, auth): ) success = False if request.method == 'POST': - form = PasswordForm(request.POST) + form = PasswordForm(user=user, data=request.POST) if form.is_valid(): password = form.cleaned_data["password"] @@ -538,7 +538,7 @@ def confirm_password_reset(request, auth): success = True else: - form = PasswordForm() + form = PasswordForm(user=user) hlibname, hashername = settings.PASSWORD_HASHERS[0].rsplit('.',1) hlib = importlib.import_module(hlibname) @@ -669,7 +669,7 @@ def change_password(request): if request.method == 'POST': form = ChangePasswordForm(user, request.POST) if form.is_valid(): - new_password = form.cleaned_data["new_password"] + new_password = form.cleaned_data["password"] user.set_password(new_password) user.save()