From 268d4d9a179b4e687ed0ff55d0b0d28e555d86d2 Mon Sep 17 00:00:00 2001 From: Mad Price Ball Date: Thu, 14 May 2020 15:29:28 -0700 Subject: [PATCH] Change password form fixes and clean up (#1111) * Add set password form (fixes #1110) * Simplify and minimize django-allauth customization * Use standard Django password validator for min length --- open_humans/account_views.py | 99 +++++-------------- open_humans/forms.py | 48 +-------- open_humans/settings.py | 6 ++ .../templates/account/password_change.html | 2 +- ...oken.html => password_reset_from_key.html} | 0 .../templates/account/password_set.html | 22 +++++ .../templates/member/my-member-settings.html | 2 +- open_humans/tests.py | 8 +- open_humans/urls.py | 16 +-- 9 files changed, 70 insertions(+), 133 deletions(-) rename open_humans/templates/account/{password_reset_token.html => password_reset_from_key.html} (100%) create mode 100644 open_humans/templates/account/password_set.html diff --git a/open_humans/account_views.py b/open_humans/account_views.py index 45719b5e3..2d3e54148 100644 --- a/open_humans/account_views.py +++ b/open_humans/account_views.py @@ -7,14 +7,10 @@ from django.shortcuts import redirect from django.urls import reverse, reverse_lazy from django.views.generic import View -from django.views.generic.edit import DeleteView, FormView +from django.views.generic.edit import DeleteView import allauth.account.app_settings as allauth_settings -from allauth.account.forms import ( - AddEmailForm as AllauthAddEmailForm, - default_token_generator, -) -from allauth.account.utils import perform_login, url_str_to_user_pk +from allauth.account.forms import AddEmailForm as AllauthAddEmailForm from allauth.account.models import EmailAddress from allauth.account.views import ( ConfirmEmailView as AllauthConfirmEmailView, @@ -22,6 +18,8 @@ EmailView as AllauthEmailView, PasswordChangeView as AllauthPasswordChangeView, PasswordResetView as AllauthPasswordResetView, + PasswordResetFromKeyView as AllauthPasswordResetFromKeyView, + PasswordSetView as AllauthPasswordSetView, SignupView as AllauthSignupView, ) from allauth.socialaccount.helpers import complete_social_login @@ -32,14 +30,14 @@ from common.mixins import PrivateMixin from .forms import ( - ChangePasswordForm, MemberLoginForm, MemberSignupForm, - PasswordResetForm, ResetPasswordForm, SocialSignupForm, ) -from .models import User, Member +from .models import Member + +User = get_user_model() class MemberLoginView(AllauthLoginView): @@ -134,90 +132,41 @@ def get_object(self, queryset=None): class ResetPasswordView(AllauthPasswordResetView): """ - Ooops, we've done lost our password, Martha! Subclasses Allauth's - view to use our template and use our own form which preserves the + Subclasses Allauth's to use our own form which preserves the next url for when the password reset process is complete. """ - template_name = "account/password_reset.html" form_class = ResetPasswordForm - success_url = reverse_lazy("account_reset_password_done") -class PasswordResetFromKeyView(FormView): +class PasswordResetFromKeyView(AllauthPasswordResetFromKeyView): """ - Let's get a new password! Allauth tries to be fancy with ajax, - but we don't really use ajax ourselves, so this view does the work - of getting the key and calling the check functions directly. + Override form_valid to use stored redirect on login. """ - template_name = "account/password_reset_token.html" - form_class = PasswordResetForm + def form_valid(self, form): + default_return = super().form_valid(form) + next_url = self.reset_user.member.password_reset_redirect + if next_url and allauth_settings.LOGIN_ON_PASSWORD_RESET: + self.reset_user.member.password_reset_redirect = "" + self.reset_user.member.save() + return redirect(next_url) + return default_return - def _get_user(self, uidb36): - User = get_user_model() - try: - pk = url_str_to_user_pk(uidb36) - return User.objects.get(pk=pk) - except (ValueError, User.DoesNotExist): - return None - - def dispatch(self, request, uidb36, key, **kwargs): - self.request = request - self.key = key - self.reset_user = self._get_user(uidb36) - if self.reset_user is None: - return redirect("account-password-reset-fail") - token = default_token_generator.check_token(self.reset_user, key) - if not token: - return redirect("account-password-reset-fail") - ret = super().dispatch(request, uidb36, key, **kwargs) - return ret - def get_context_data(self, **kwargs): - ret = super().get_context_data(**kwargs) - ret["action_url"] = reverse( - "account_reset_password_from_key", - kwargs={"uidb36": self.kwargs["uidb36"], "key": self.kwargs["key"]}, - ) - return ret - - def change_password(self, password): - user = self.reset_user - user.set_password(password) - user.save() - return user +class PasswordSetView(AllauthPasswordSetView): + """ + Change the success_url for password set. + """ - def form_valid(self, form): - if form.is_valid(): - self.change_password(form.clean_password()) - - if allauth_settings.LOGIN_ON_PASSWORD_RESET: - perform_login( - self.request, - self.reset_user, - email_verification=allauth_settings.EMAIL_VERIFICATION, - ) - member = Member.objects.get(user=self.reset_user) - next_url = member.password_reset_redirect - member.password_reset_redirect = "" # Clear redirect from db - member.save() - messages = { - "settings_updated": { - "level": django_messages.SUCCESS, - "text": "Password successfully reset.", - } - } - return redirect(next_url) + success_url = reverse_lazy("my-member-settings") class PasswordChangeView(AllauthPasswordChangeView): """ - Change the password, subclass allauth to use our own template + Change success_url for password change. """ - template_name = "account/password_change.html" - form_class = ChangePasswordForm success_url = reverse_lazy("my-member-settings") diff --git a/open_humans/forms.py b/open_humans/forms.py index 5652cb157..87b160692 100644 --- a/open_humans/forms.py +++ b/open_humans/forms.py @@ -13,6 +13,7 @@ LoginForm as AllauthLoginForm, ResetPasswordForm as AllauthResetPasswordForm, SignupForm as AllauthSignupForm, + SetPasswordForm as AllauthSetPasswordForm, ) from allauth.account.models import EmailAddress from allauth.account.utils import filter_users_by_email @@ -69,9 +70,8 @@ class MemberSignupForm(AllauthSignupForm): """ A subclass of django-allauth's SignupForm with additions. - A `terms` field is added for the Terms of Use checkbox, a `name` field - is added to store a Member's username, and additional validation is - added for passwords to impose a minimum length. + A `terms` field is added for the Terms of Use checkbox, and a `name` field + is added to store a Member's username. """ name = forms.CharField(max_length=30) @@ -80,48 +80,6 @@ class MemberSignupForm(AllauthSignupForm): class Meta: # noqa: D101 fields = "__all__" - def clean_password(self): - return _clean_password(AllauthSignupForm, self, "password") - - -class ChangePasswordForm(AllauthChangePasswordForm): - """ - A subclass of account's ChangePasswordForm that checks password length. - """ - - def clean_password_new(self): - return _clean_password(ChangePasswordForm, self, "password_new") - - -class PasswordResetForm(forms.Form): - """ - Change the user's password, matches our template better than the form class - shipped by allauth. - """ - - password = forms.CharField( - label="New Password", widget=forms.PasswordInput(render_value=False) - ) - password_confirm = forms.CharField( - label="New Password (again)", widget=forms.PasswordInput(render_value=False) - ) - - def clean(self): - super().clean() - if self._errors: - return - - if "password" in self.cleaned_data and "password_confirm" in self.cleaned_data: - if self.cleaned_data["password"] != self.cleaned_data["password_confirm"]: - self.add_error( - "password_confirm", "You must type the same password each time." - ) - - return self.cleaned_data - - def clean_password(self): - return _clean_password(PasswordResetForm, self, "password") - class MemberProfileEditForm(forms.ModelForm): """ diff --git a/open_humans/settings.py b/open_humans/settings.py index e9e151294..5fa31b5ba 100644 --- a/open_humans/settings.py +++ b/open_humans/settings.py @@ -303,6 +303,12 @@ def to_bool(env, default="false"): LOGIN_REDIRECT_URL = "home" AUTH_USER_MODEL = "open_humans.User" +AUTH_PASSWORD_VALIDATORS = [ + { + "NAME": "django.contrib.auth.password_validation.MinimumLengthValidator", + "OPTIONS": {"min_length": 8}, + } +] ACCOUNT_ADAPTER = "common.adapters.MyAccountAdapter" ACCOUNT_AUTHENTICATED_LOGIN_REDIRECTS = True diff --git a/open_humans/templates/account/password_change.html b/open_humans/templates/account/password_change.html index c7760b42b..fd7331a91 100644 --- a/open_humans/templates/account/password_change.html +++ b/open_humans/templates/account/password_change.html @@ -8,7 +8,7 @@ {% block panel_content %}
+ action="{% url 'account_change_password' %}" id="password-change-form"> {% csrf_token %} diff --git a/open_humans/templates/account/password_reset_token.html b/open_humans/templates/account/password_reset_from_key.html similarity index 100% rename from open_humans/templates/account/password_reset_token.html rename to open_humans/templates/account/password_reset_from_key.html diff --git a/open_humans/templates/account/password_set.html b/open_humans/templates/account/password_set.html new file mode 100644 index 000000000..761405f09 --- /dev/null +++ b/open_humans/templates/account/password_set.html @@ -0,0 +1,22 @@ +{% extends 'panel.html' %} + +{% load bootstrap_tags %} +{% load utilities %} + +{% block head_title %}Set Password{% endblock %} + +{% block panel_content %} +
+ + {% csrf_token %} + + + + {{ form|as_bootstrap_horizontal:'col-md-4' }} + + + +
+{% endblock %} diff --git a/open_humans/templates/member/my-member-settings.html b/open_humans/templates/member/my-member-settings.html index b35a50d56..f7539d71f 100644 --- a/open_humans/templates/member/my-member-settings.html +++ b/open_humans/templates/member/my-member-settings.html @@ -79,7 +79,7 @@
Allow other members to contact you?
- Change password diff --git a/open_humans/tests.py b/open_humans/tests.py index 3742042ce..961bea834 100644 --- a/open_humans/tests.py +++ b/open_humans/tests.py @@ -89,7 +89,8 @@ class SmokeTests(SmokeTestCase): ] authenticated_urls = redirect_urls + [ - "/account/password/", + "/account/password/change/", + "/account/password/set/", ( "/oauth2/authorize/?origin=external&response_type=code" "&scope=go-viral%20read%20write&client_id=example-id-15" @@ -168,12 +169,13 @@ def test_password_reset(self): key = reset_url.split("/")[7] # Go ahead and reset the mailbox mail.outbox = [] - do_reset_response = self.client.get(reset_url) + do_reset_response = self.client.get(reset_url, follow=True) self.assertEqual(do_reset_response.status_code, 200) self.assertContains(do_reset_response, "Set your new password") do_reset_post_response = self.client.post( - reset_url, {"password": "asdfqwerty", "password_confirm": "asdfqwerty"} + do_reset_response.redirect_chain[-1][0], + {"password1": "asdfqwerty", "password2": "asdfqwerty"}, ) self.assertEqual(do_reset_post_response.status_code, 302) self.assertEqual(do_reset_post_response.url, redirect) diff --git a/open_humans/urls.py b/open_humans/urls.py index 4daceba28..f405fd9f4 100644 --- a/open_humans/urls.py +++ b/open_humans/urls.py @@ -102,22 +102,22 @@ path( "account/login/", account_views.MemberLoginView.as_view(), name="account_login" ), - # More overrides - custom forms to enforce password length minimum. + # Override to change success_url. path( - "account/password/", + "account/password/set/", + account_views.PasswordSetView.as_view(), + name="account_set_password", + ), + path( + "account/password/change/", account_views.PasswordChangeView.as_view(), - name="account_password", + name="account_change_password", ), re_path( r"^account/confirm-email/(?P[-:\w]+)/$", account_views.ConfirmEmailView.as_view(), name="account_confirm_email", ), - path( - "account/password/change/", - account_views.PasswordChangeView.as_view(), - name="account_change_password", - ), path( "account/password/reset/done/", TemplateView.as_view(template_name="account/password_reset_sent.html"),