From 4a65ee326adb168138fd6aee9793e2a89a98efc4 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Fri, 15 Jul 2022 09:59:57 -0700 Subject: [PATCH 1/7] Use password validation in register view --- bookwyrm/forms/landing.py | 6 ++++++ bookwyrm/tests/views/landing/test_register.py | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/bookwyrm/forms/landing.py b/bookwyrm/forms/landing.py index b01c2cc98c..36c18dfb86 100644 --- a/bookwyrm/forms/landing.py +++ b/bookwyrm/forms/landing.py @@ -1,4 +1,6 @@ """ Forms for the landing pages """ +from django.contrib.auth.password_validation import validate_password +from django.core.exceptions import ValidationError from django.forms import PasswordInput from django.utils.translation import gettext_lazy as _ @@ -28,6 +30,10 @@ 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")) diff --git a/bookwyrm/tests/views/landing/test_register.py b/bookwyrm/tests/views/landing/test_register.py index 24360a646e..aa1ca7fb90 100644 --- a/bookwyrm/tests/views/landing/test_register.py +++ b/bookwyrm/tests/views/landing/test_register.py @@ -122,6 +122,17 @@ def test_register_invalid_email(self, *_): 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() From 659ee96002e06b7be0bee90adbc96a399fcd6aa2 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Fri, 15 Jul 2022 10:45:08 -0700 Subject: [PATCH 2/7] 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 | 27 ++++++++++++--- bookwyrm/views/preferences/change_password.py | 26 +++++---------- 4 files changed, 69 insertions(+), 44 deletions(-) diff --git a/bookwyrm/forms/edit_user.py b/bookwyrm/forms/edit_user.py index d609f15dcc..a291c64416 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 ad34aca1ac..3b816779d3 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 b6d2f48ef1..fdcf577b0c 100644 --- a/bookwyrm/tests/views/preferences/test_change_password.py +++ b/bookwyrm/tests/views/preferences/test_change_password.py @@ -46,8 +46,8 @@ def test_password_change(self): "", { "current_password": "password", - "password": "hi", - "confirm-password": "hi", + "password": "longwordsecure", + "confirm_password": "longwordsecure", }, ) request.user = self.local_user @@ -64,8 +64,8 @@ def test_password_change_wrong_current(self): "", { "current_password": "not my password", - "password": "hi", - "confirm-password": "hihi", + "password": "longwordsecure", + "confirm_password": "hihi", }, ) request.user = self.local_user @@ -74,6 +74,23 @@ def test_password_change_wrong_current(self): self.assertEqual(self.local_user.password, password_hash) def test_password_change_mismatch(self): + """change password""" + view = views.ChangePassword.as_view() + password_hash = self.local_user.password + request = self.factory.post( + "", + { + "current_password": "password", + "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 @@ -82,7 +99,7 @@ def test_password_change_mismatch(self): { "current_password": "password", "password": "hi", - "confirm-password": "hihi", + "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 eaca2d8fa1..1563cd08a7 100644 --- a/bookwyrm/views/preferences/change_password.py +++ b/bookwyrm/views/preferences/change_password.py @@ -3,11 +3,10 @@ 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) From b62f8eff42e402c7c7aa1b3a353c7e893c90b61c Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Fri, 15 Jul 2022 10:56:13 -0700 Subject: [PATCH 3/7] Updates method decorators --- bookwyrm/views/preferences/change_password.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bookwyrm/views/preferences/change_password.py b/bookwyrm/views/preferences/change_password.py index 1563cd08a7..d660355604 100644 --- a/bookwyrm/views/preferences/change_password.py +++ b/bookwyrm/views/preferences/change_password.py @@ -19,10 +19,10 @@ def get(self, request): data = {"form": forms.ChangePasswordForm()} return TemplateResponse(request, "preferences/change_password.html", data) - @sensitive_variables("new_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""" form = forms.ChangePasswordForm(request.POST, instance=request.user) From 1bb0a9d99864c6573bce0e410e6521872f1c77bd Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Fri, 15 Jul 2022 11:18:47 -0700 Subject: [PATCH 4/7] Updates tests --- bookwyrm/tests/views/preferences/test_change_password.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/bookwyrm/tests/views/preferences/test_change_password.py b/bookwyrm/tests/views/preferences/test_change_password.py index fdcf577b0c..879ffd03d6 100644 --- a/bookwyrm/tests/views/preferences/test_change_password.py +++ b/bookwyrm/tests/views/preferences/test_change_password.py @@ -54,6 +54,7 @@ def test_password_change(self): 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): @@ -71,6 +72,7 @@ def test_password_change_wrong_current(self): 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): @@ -88,6 +90,7 @@ def test_password_change_mismatch(self): 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): @@ -105,4 +108,5 @@ def test_password_change_invalid(self): 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) From 3846b201bd153eb657efe8942a3cc49882341d1d Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Fri, 15 Jul 2022 11:25:49 -0700 Subject: [PATCH 5/7] Updates reset password flow to use validators --- bookwyrm/forms/landing.py | 31 +++++++++++++++++-- .../templates/landing/password_reset.html | 6 ++-- bookwyrm/views/landing/password.py | 15 +++++---- 3 files changed, 39 insertions(+), 13 deletions(-) diff --git a/bookwyrm/forms/landing.py b/bookwyrm/forms/landing.py index 36c18dfb86..a31e8a7c44 100644 --- a/bookwyrm/forms/landing.py +++ b/bookwyrm/forms/landing.py @@ -1,7 +1,7 @@ """ Forms for the landing pages """ +from django import forms from django.contrib.auth.password_validation import validate_password from django.core.exceptions import ValidationError -from django.forms import PasswordInput from django.utils.translation import gettext_lazy as _ from bookwyrm import models @@ -15,7 +15,7 @@ class Meta: fields = ["localname", "password"] help_texts = {f: None for f in fields} widgets = { - "password": PasswordInput(), + "password": forms.PasswordInput(), } @@ -24,7 +24,7 @@ class Meta: 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""" @@ -49,3 +49,28 @@ def clean(self): 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 8348efd4f7..d56cba6254 100644 --- a/bookwyrm/templates/landing/password_reset.html +++ b/bookwyrm/templates/landing/password_reset.html @@ -26,7 +26,8 @@

{% trans "Reset Password" %}

{% trans "Password:" %}
- + {{ form.password }} + {% include 'snippets/form_errors.html' with errors_list=form.password.errors id="desc_current_password" %}
@@ -34,7 +35,8 @@

{% trans "Reset Password" %}

{% 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/views/landing/password.py b/bookwyrm/views/landing/password.py index a7eb001b00..7487b94144 100644 --- a/bookwyrm/views/landing/password.py +++ b/bookwyrm/views/landing/password.py @@ -5,7 +5,7 @@ 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 @@ def get(self, request, code): 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 @@ def post(self, request, code): 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) From b1f5171502df49cb7847005f70c2cd8db57f9152 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Fri, 15 Jul 2022 11:39:29 -0700 Subject: [PATCH 6/7] Updates reset password tests --- bookwyrm/tests/views/landing/test_password.py | 21 ++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/bookwyrm/tests/views/landing/test_password.py b/bookwyrm/tests/views/landing/test_password.py index b1f7e59f04..c7c7e05d52 100644 --- a/bookwyrm/tests/views/landing/test_password.py +++ b/bookwyrm/tests/views/landing/test_password.py @@ -104,7 +104,9 @@ def test_password_reset_post(self): """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 @@ def test_password_reset_wrong_code(self): """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 @@ def test_password_reset_mismatch(self): """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()) From 65117fe3c6bc3e8567a9576c7b66e52258fa8a9b Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Fri, 15 Jul 2022 11:41:39 -0700 Subject: [PATCH 7/7] Use manual password field to customize id --- bookwyrm/templates/landing/password_reset.html | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/bookwyrm/templates/landing/password_reset.html b/bookwyrm/templates/landing/password_reset.html index d56cba6254..786eaa0ab7 100644 --- a/bookwyrm/templates/landing/password_reset.html +++ b/bookwyrm/templates/landing/password_reset.html @@ -26,8 +26,16 @@

{% trans "Reset Password" %}

{% trans "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=form.password.errors id="desc_password" %}