Merge pull request #2229 from bookwyrm-social/password-validation

Password validation

bookwyrm/forms/edit_user.py

@@ -1,5 +1,8 @@
 """ using django model forms """
 from django import forms
+from django.contrib.auth.password_validation import validate_password
+from django.core.exceptions import ValidationError
+from django.utils.translation import gettext_lazy as _
 
 from bookwyrm import models
 from bookwyrm.models.fields import ClearableFileInputWithWarning
@@ -66,3 +69,33 @@ class DeleteUserForm(CustomForm):
     class Meta:
         model = models.User
         fields = ["password"]
+
+
+class ChangePasswordForm(CustomForm):
+    current_password = forms.CharField(widget=forms.PasswordInput)
+    confirm_password = forms.CharField(widget=forms.PasswordInput)
+
+    class Meta:
+        model = models.User
+        fields = ["password"]
+        widgets = {
+            "password": forms.PasswordInput(),
+        }
+
+    def clean(self):
+        """Make sure passwords match and are valid"""
+        current_password = self.data.get("current_password")
+        if not self.instance.check_password(current_password):
+            self.add_error("current_password", _("Incorrect password"))
+
+        cleaned_data = super().clean()
+        new_password = cleaned_data.get("password")
+        confirm_password = self.data.get("confirm_password")
+
+        if new_password != confirm_password:
+            self.add_error("confirm_password", _("Password does not match"))
+
+        try:
+            validate_password(new_password)
+        except ValidationError as err:
+            self.add_error("password", err)

bookwyrm/forms/landing.py

@@ -1,5 +1,7 @@
 """ Forms for the landing pages """
-from django.forms import PasswordInput
+from django import forms
+from django.contrib.auth.password_validation import validate_password
+from django.core.exceptions import ValidationError
 from django.utils.translation import gettext_lazy as _
 
 from bookwyrm import models
@@ -13,7 +15,7 @@ class LoginForm(CustomForm):
         fields = ["localname", "password"]
         help_texts = {f: None for f in fields}
         widgets = {
-            "password": PasswordInput(),
+            "password": forms.PasswordInput(),
         }
 
 
@@ -22,12 +24,16 @@ class RegisterForm(CustomForm):
         model = models.User
         fields = ["localname", "email", "password"]
         help_texts = {f: None for f in fields}
-        widgets = {"password": PasswordInput()}
+        widgets = {"password": forms.PasswordInput()}
 
     def clean(self):
         """Check if the username is taken"""
         cleaned_data = super().clean()
         localname = cleaned_data.get("localname").strip()
+        try:
+            validate_password(cleaned_data.get("password"))
+        except ValidationError as err:
+            self.add_error("password", err)
         if models.User.objects.filter(localname=localname).first():
             self.add_error("localname", _("User with this username already exists"))
 
@@ -43,3 +49,28 @@ class InviteRequestForm(CustomForm):
     class Meta:
         model = models.InviteRequest
         fields = ["email", "answer"]
+
+
+class PasswordResetForm(CustomForm):
+    confirm_password = forms.CharField(widget=forms.PasswordInput)
+
+    class Meta:
+        model = models.User
+        fields = ["password"]
+        widgets = {
+            "password": forms.PasswordInput(),
+        }
+
+    def clean(self):
+        """Make sure the passwords match and are valid"""
+        cleaned_data = super().clean()
+        new_password = cleaned_data.get("password")
+        confirm_password = self.data.get("confirm_password")
+
+        if new_password != confirm_password:
+            self.add_error("confirm_password", _("Password does not match"))
+
+        try:
+            validate_password(new_password)
+        except ValidationError as err:
+            self.add_error("password", err)

bookwyrm/templates/landing/password_reset.html

@@ -26,7 +26,16 @@
                         {% trans "Password:" %}
                     </label>
                     <div class="control">
-                        <input type="password" name="password" maxlength="128" class="input" required="" id="id_new_password" aria-describedby="form_errors">
+                        <input
+                            type="password"
+                            name="password"
+                            maxlength="128"
+                            class="input"
+                            required=""
+                            id="id_new_password"
+                            aria-describedby="desc_password"
+                        >
+                        {% include 'snippets/form_errors.html' with errors_list=form.password.errors id="desc_password" %}
                     </div>
                 </div>
                 <div class="field">
@@ -34,7 +43,8 @@
                         {% trans "Confirm password:" %}
                     </label>
                     <div class="control">
-                        <input type="password" name="confirm-password" maxlength="128" class="input" required="" id="id_confirm_password" aria-describedby="form_errors">
+                        {{ form.confirm_password }}
+                        {% include 'snippets/form_errors.html' with errors_list=form.confirm_password.errors id="desc_confirm_password" %}
                     </div>
                 </div>
                 <div class="field is-grouped">

bookwyrm/templates/preferences/change_password.html

@@ -20,34 +20,19 @@
     {% csrf_token %}
     <div class="field">
         <label class="label" for="id_password">{% trans "Current password:" %}</label>
-        <input
-            type="password"
-            name="current_password"
-            maxlength="128"
-            class="input"
-            required=""
-            id="id_current_password"
-            aria-describedby="desc_current_password"
-        >
-        {% include 'snippets/form_errors.html' with errors_list=errors.current_password id="desc_current_password" %}
+        {{ form.current_password }}
+        {% include 'snippets/form_errors.html' with errors_list=form.current_password.errors id="desc_current_password" %}
     </div>
     <hr aria-hidden="true" />
     <div class="field">
         <label class="label" for="id_password">{% trans "New password:" %}</label>
-        <input type="password" name="password" maxlength="128" class="input" required="" id="id_password">
+        {{ form.password }}
+        {% include 'snippets/form_errors.html' with errors_list=form.password.errors id="desc_current_password" %}
     </div>
     <div class="field">
         <label class="label" for="id_confirm_password">{% trans "Confirm password:" %}</label>
-        <input
-            type="password"
-            name="confirm-password"
-            maxlength="128"
-            class="input"
-            required=""
-            id="id_confirm_password"
-            aria-describedby="desc_confirm_password"
-        >
-        {% include 'snippets/form_errors.html' with errors_list=errors.confirm_password id="desc_confirm_password" %}
+        {{ form.confirm_password }}
+        {% include 'snippets/form_errors.html' with errors_list=form.confirm_password.errors id="desc_confirm_password" %}
     </div>
     <button class="button is-primary" type="submit">{% trans "Change Password" %}</button>
 </form>

bookwyrm/tests/views/landing/test_password.py

@@ -104,7 +104,9 @@ class PasswordViews(TestCase):
         """reset from code"""
         view = views.PasswordReset.as_view()
         code = models.PasswordReset.objects.create(user=self.local_user)
-        request = self.factory.post("", {"password": "hi", "confirm-password": "hi"})
+        request = self.factory.post(
+            "", {"password": "longwordsecure", "confirm_password": "longwordsecure"}
+        )
         with patch("bookwyrm.views.landing.password.login"):
             resp = view(request, code.code)
         self.assertEqual(resp.status_code, 302)
@@ -114,7 +116,9 @@ class PasswordViews(TestCase):
         """reset from code"""
         view = views.PasswordReset.as_view()
         models.PasswordReset.objects.create(user=self.local_user)
-        request = self.factory.post("", {"password": "hi", "confirm-password": "hi"})
+        request = self.factory.post(
+            "", {"password": "longwordsecure", "confirm_password": "longwordsecure"}
+        )
         resp = view(request, "jhgdkfjgdf")
         validate_html(resp.render())
         self.assertTrue(models.PasswordReset.objects.exists())
@@ -123,7 +127,18 @@ class PasswordViews(TestCase):
         """reset from code"""
         view = views.PasswordReset.as_view()
         code = models.PasswordReset.objects.create(user=self.local_user)
-        request = self.factory.post("", {"password": "hi", "confirm-password": "hihi"})
+        request = self.factory.post(
+            "", {"password": "longwordsecure", "confirm_password": "hihi"}
+        )
+        resp = view(request, code.code)
+        validate_html(resp.render())
+        self.assertTrue(models.PasswordReset.objects.exists())
+
+    def test_password_reset_invalid(self):
+        """reset from code"""
+        view = views.PasswordReset.as_view()
+        code = models.PasswordReset.objects.create(user=self.local_user)
+        request = self.factory.post("", {"password": "a", "confirm_password": "a"})
         resp = view(request, code.code)
         validate_html(resp.render())
         self.assertTrue(models.PasswordReset.objects.exists())

bookwyrm/tests/views/landing/test_register.py

@@ -122,6 +122,17 @@ class RegisterViews(TestCase):
         self.assertEqual(models.User.objects.count(), 1)
         validate_html(response.render())
 
+    def test_register_invalid_password(self, *_):
+        """gotta have an email"""
+        view = views.Register.as_view()
+        self.assertEqual(models.User.objects.count(), 1)
+        request = self.factory.post(
+            "register/", {"localname": "nutria", "password": "password", "email": "aa"}
+        )
+        response = view(request)
+        self.assertEqual(models.User.objects.count(), 1)
+        validate_html(response.render())
+
     def test_register_error_and_invite(self, *_):
         """redirect to the invite page"""
         view = views.Register.as_view()

bookwyrm/tests/views/preferences/test_change_password.py

@@ -46,14 +46,15 @@ class ChangePasswordViews(TestCase):
             "",
             {
                 "current_password": "password",
-                "password": "hi",
-                "confirm-password": "hi",
+                "password": "longwordsecure",
+                "confirm_password": "longwordsecure",
             },
         )
         request.user = self.local_user
         with patch("bookwyrm.views.preferences.change_password.login"):
             result = view(request)
         validate_html(result.render())
+        self.local_user.refresh_from_db()
         self.assertNotEqual(self.local_user.password, password_hash)
 
     def test_password_change_wrong_current(self):
@@ -64,13 +65,14 @@ class ChangePasswordViews(TestCase):
             "",
             {
                 "current_password": "not my password",
-                "password": "hi",
-                "confirm-password": "hihi",
+                "password": "longwordsecure",
+                "confirm_password": "hihi",
             },
         )
         request.user = self.local_user
         result = view(request)
         validate_html(result.render())
+        self.local_user.refresh_from_db()
         self.assertEqual(self.local_user.password, password_hash)
 
     def test_password_change_mismatch(self):
@@ -81,11 +83,30 @@ class ChangePasswordViews(TestCase):
             "",
             {
                 "current_password": "password",
-                "password": "hi",
-                "confirm-password": "hihi",
+                "password": "longwordsecure",
+                "confirm_password": "hihi",
             },
         )
         request.user = self.local_user
         result = view(request)
         validate_html(result.render())
+        self.local_user.refresh_from_db()
+        self.assertEqual(self.local_user.password, password_hash)
+
+    def test_password_change_invalid(self):
+        """change password"""
+        view = views.ChangePassword.as_view()
+        password_hash = self.local_user.password
+        request = self.factory.post(
+            "",
+            {
+                "current_password": "password",
+                "password": "hi",
+                "confirm_password": "hi",
+            },
+        )
+        request.user = self.local_user
+        result = view(request)
+        validate_html(result.render())
+        self.local_user.refresh_from_db()
         self.assertEqual(self.local_user.password, password_hash)

bookwyrm/views/landing/password.py

@@ -5,7 +5,7 @@ from django.shortcuts import redirect
 from django.template.response import TemplateResponse
 from django.views import View
 
-from bookwyrm import models
+from bookwyrm import forms, models
 from bookwyrm.emailing import password_reset_email
 
 
@@ -57,7 +57,8 @@ class PasswordReset(View):
         except models.PasswordReset.DoesNotExist:
             raise PermissionDenied()
 
-        return TemplateResponse(request, "landing/password_reset.html", {"code": code})
+        data = {"code": code, "form": forms.PasswordResetForm()}
+        return TemplateResponse(request, "landing/password_reset.html", data)
 
     def post(self, request, code):
         """allow a user to change their password through an emailed token"""
@@ -68,14 +69,12 @@ class PasswordReset(View):
             return TemplateResponse(request, "landing/password_reset.html", data)
 
         user = reset_code.user
-
-        new_password = request.POST.get("password")
-        confirm_password = request.POST.get("confirm-password")
-
-        if new_password != confirm_password:
-            data = {"errors": ["Passwords do not match"]}
+        form = forms.PasswordResetForm(request.POST, instance=user)
+        if not form.is_valid():
+            data = {"code": code, "form": form}
             return TemplateResponse(request, "landing/password_reset.html", data)
 
+        new_password = form.cleaned_data["password"]
         user.set_password(new_password)
         user.save(broadcast=False, update_fields=["password"])
         login(request, user)

bookwyrm/views/preferences/change_password.py

@@ -3,11 +3,10 @@ from django.contrib.auth import login
 from django.contrib.auth.decorators import login_required
 from django.template.response import TemplateResponse
 from django.utils.decorators import method_decorator
-from django.utils.translation import gettext_lazy as _
 from django.views import View
 from django.views.decorators.debug import sensitive_variables, sensitive_post_parameters
 
-from bookwyrm import models
+from bookwyrm import forms
 
 
 # pylint: disable= no-self-use
@@ -17,33 +16,24 @@ class ChangePassword(View):
 
     def get(self, request):
         """change password page"""
-        data = {"user": request.user}
+        data = {"form": forms.ChangePasswordForm()}
         return TemplateResponse(request, "preferences/change_password.html", data)
 
-    @sensitive_variables("new_password")
-    @sensitive_variables("confirm_password")
+    @method_decorator(sensitive_variables("new_password"))
     @method_decorator(sensitive_post_parameters("current_password"))
     @method_decorator(sensitive_post_parameters("password"))
-    @method_decorator(sensitive_post_parameters("confirm__password"))
+    @method_decorator(sensitive_post_parameters("confirm_password"))
     def post(self, request):
         """allow a user to change their password"""
-        data = {"user": request.user}
-
-        # check current password
-        user = models.User.objects.get(id=request.user.id)
-        if not user.check_password(request.POST.get("current_password")):
-            data["errors"] = {"current_password": [_("Incorrect password")]}
-            return TemplateResponse(request, "preferences/change_password.html", data)
-
-        new_password = request.POST.get("password")
-        confirm_password = request.POST.get("confirm-password")
-
-        if new_password != confirm_password:
-            data["errors"] = {"confirm_password": [_("Password does not match")]}
+        form = forms.ChangePasswordForm(request.POST, instance=request.user)
+        if not form.is_valid():
+            data = {"form": form}
             return TemplateResponse(request, "preferences/change_password.html", data)
 
+        new_password = form.cleaned_data["password"]
         request.user.set_password(new_password)
         request.user.save(broadcast=False, update_fields=["password"])
+
         login(request, request.user)
-        data["success"] = True
+        data = {"success": True, "form": forms.ChangePasswordForm()}
         return TemplateResponse(request, "preferences/change_password.html", data)