fix: improve pw validation without js (#9113)

* fix: backend validators for PasswordForm

Only used for password reset

* refactor: share more code

* fix: reconcile with PersonPasswordForm

This form class hierarchy reallllly needs to be refactored more deeply

* fix: attach pw errors to specific fields

* fix: return val from clean_password_confirmation

* refactor: share PasswordForm code

* test: update test_change_password

* test: update test_create_existing_account

* test: update test_reset_password

Author: jennifer-richards

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=[('-','--------')])

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)

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()