From 659ee96002e06b7be0bee90adbc96a399fcd6aa2 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Fri, 15 Jul 2022 10:45:08 -0700 Subject: [PATCH] Use password validation in change password flow This also moves the form validation into a form instead of doing it in the view. --- bookwyrm/forms/edit_user.py | 33 +++++++++++++++++++ .../preferences/change_password.html | 27 ++++----------- .../views/preferences/test_change_password.py | 29 ++++++++++++---- bookwyrm/views/preferences/change_password.py | 26 +++++---------- 4 files changed, 70 insertions(+), 45 deletions(-) diff --git a/bookwyrm/forms/edit_user.py b/bookwyrm/forms/edit_user.py index d609f15dc..a291c6441 100644 --- a/bookwyrm/forms/edit_user.py +++ b/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) diff --git a/bookwyrm/templates/preferences/change_password.html b/bookwyrm/templates/preferences/change_password.html index ad34aca1a..3b816779d 100644 --- a/bookwyrm/templates/preferences/change_password.html +++ b/bookwyrm/templates/preferences/change_password.html @@ -20,34 +20,19 @@ {% csrf_token %}
- - {% 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" %}
- + {{ form.password }} + {% include 'snippets/form_errors.html' with errors_list=form.password.errors id="desc_current_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" %}
diff --git a/bookwyrm/tests/views/preferences/test_change_password.py b/bookwyrm/tests/views/preferences/test_change_password.py index b6d2f48ef..fdcf577b0 100644 --- a/bookwyrm/tests/views/preferences/test_change_password.py +++ b/bookwyrm/tests/views/preferences/test_change_password.py @@ -46,8 +46,8 @@ class ChangePasswordViews(TestCase): "", { "current_password": "password", - "password": "hi", - "confirm-password": "hi", + "password": "longwordsecure", + "confirm_password": "longwordsecure", }, ) request.user = self.local_user @@ -64,8 +64,8 @@ class ChangePasswordViews(TestCase): "", { "current_password": "not my password", - "password": "hi", - "confirm-password": "hihi", + "password": "longwordsecure", + "confirm_password": "hihi", }, ) request.user = self.local_user @@ -81,8 +81,25 @@ 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.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 diff --git a/bookwyrm/views/preferences/change_password.py b/bookwyrm/views/preferences/change_password.py index eaca2d8fa..1563cd08a 100644 --- a/bookwyrm/views/preferences/change_password.py +++ b/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_post_parameters("current_password")) @method_decorator(sensitive_post_parameters("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)