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/forms/landing.py b/bookwyrm/forms/landing.py index b01c2cc98..a31e8a7c4 100644 --- a/bookwyrm/forms/landing.py +++ b/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) diff --git a/bookwyrm/templates/landing/password_reset.html b/bookwyrm/templates/landing/password_reset.html index 8348efd4f..786eaa0ab 100644 --- a/bookwyrm/templates/landing/password_reset.html +++ b/bookwyrm/templates/landing/password_reset.html @@ -26,7 +26,16 @@ {% trans "Password:" %}
- + + {% include 'snippets/form_errors.html' with errors_list=form.password.errors id="desc_password" %}
@@ -34,7 +43,8 @@ {% trans "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/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/landing/test_password.py b/bookwyrm/tests/views/landing/test_password.py index b1f7e59f0..c7c7e05d5 100644 --- a/bookwyrm/tests/views/landing/test_password.py +++ b/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()) diff --git a/bookwyrm/tests/views/landing/test_register.py b/bookwyrm/tests/views/landing/test_register.py index 24360a646..aa1ca7fb9 100644 --- a/bookwyrm/tests/views/landing/test_register.py +++ b/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() diff --git a/bookwyrm/tests/views/preferences/test_change_password.py b/bookwyrm/tests/views/preferences/test_change_password.py index b6d2f48ef..879ffd03d 100644 --- a/bookwyrm/tests/views/preferences/test_change_password.py +++ b/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) diff --git a/bookwyrm/views/landing/password.py b/bookwyrm/views/landing/password.py index a7eb001b0..7487b9414 100644 --- a/bookwyrm/views/landing/password.py +++ b/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) diff --git a/bookwyrm/views/preferences/change_password.py b/bookwyrm/views/preferences/change_password.py index eaca2d8fa..d66035560 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_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)