From 2827f545b18dc339f7a856d003c8090e244968de Mon Sep 17 00:00:00 2001 From: Aarif Date: Fri, 10 Jan 2020 02:37:14 +0500 Subject: [PATCH] changes for password_reset_confirm deprecated view --- .../core/djangoapps/user_authn/urls_common.py | 8 +- .../user_authn/views/password_reset.py | 292 +++++++++++------- .../views/tests/test_reset_password.py | 43 ++- 3 files changed, 207 insertions(+), 136 deletions(-) diff --git a/openedx/core/djangoapps/user_authn/urls_common.py b/openedx/core/djangoapps/user_authn/urls_common.py index 02a0c0d4b1..f51c789a9b 100644 --- a/openedx/core/djangoapps/user_authn/urls_common.py +++ b/openedx/core/djangoapps/user_authn/urls_common.py @@ -13,10 +13,10 @@ which leads to inconsistent prefixing. from django.conf import settings from django.conf.urls import url -from django.contrib.auth.views import password_reset_complete +from django.contrib.auth.views import PasswordResetCompleteView from .views import auto_auth, login, logout, password_reset, register - +from .views.password_reset import PasswordResetConfirmWrapper urlpatterns = [ # Registration @@ -55,7 +55,7 @@ urlpatterns = [ url(r'^password_reset/$', password_reset.password_reset, name='password_reset'), url( r'^password_reset_confirm/(?P[0-9A-Za-z]+)-(?P.+)/$', - password_reset.password_reset_confirm_wrapper, + PasswordResetConfirmWrapper.as_view(), name='password_reset_confirm', ), url(r'^account/password$', password_reset.password_change_request_handler, name='password_change_request'), @@ -66,7 +66,7 @@ urlpatterns = [ urlpatterns += [ url( r'^password_reset_complete/$', - password_reset_complete, + PasswordResetCompleteView.as_view(), name='password_reset_complete', ), ] diff --git a/openedx/core/djangoapps/user_authn/views/password_reset.py b/openedx/core/djangoapps/user_authn/views/password_reset.py index fdc8ed8cbf..0289cb07ce 100644 --- a/openedx/core/djangoapps/user_authn/views/password_reset.py +++ b/openedx/core/djangoapps/user_authn/views/password_reset.py @@ -1,19 +1,21 @@ """ Password reset logic and views . """ import logging + from django import forms -from django.contrib.auth.forms import PasswordResetForm +from django.contrib.auth.forms import PasswordResetForm, SetPasswordForm from django.conf import settings from django.contrib import messages from django.contrib.auth.hashers import UNUSABLE_PASSWORD_PREFIX from django.contrib.auth.models import User from django.contrib.auth.tokens import default_token_generator -from django.contrib.auth.views import password_reset_confirm +from django.contrib.auth.views import ( + INTERNAL_RESET_SESSION_TOKEN, PasswordResetConfirmView) from django.core.exceptions import ObjectDoesNotExist from django.core.validators import ValidationError -from django.http import Http404, HttpResponse, HttpResponseBadRequest, HttpResponseForbidden +from django.http import Http404, HttpResponse, HttpResponseBadRequest, HttpResponseForbidden, HttpResponseRedirect from django.template.response import TemplateResponse from django.utils.decorators import method_decorator from django.utils.encoding import force_bytes, force_text @@ -50,7 +52,6 @@ from util.json_request import JsonResponse from util.password_policy_validators import normalize_password, validate_password from util.request_rate_limiter import BadRequestRateLimiter, PasswordResetEmailRateLimiter - SETTING_CHANGE_INITIATED = 'edx.user.settings.change_initiated' # Maintaining this naming for backwards compatibility. @@ -159,7 +160,7 @@ class PasswordResetFormNoActive(PasswordResetForm): Validates that a user exists with the given email address. """ email = self.cleaned_data["email"] - #The line below contains the only change, removing is_active=True + # The line below contains the only change, removing is_active=True self.users_cache = User.objects.filter(email__iexact=email) if not self.users_cache and is_secondary_email_feature_enabled(): @@ -299,62 +300,158 @@ def _uidb36_to_uidb64(uidb36): return uidb64 -# pylint: disable=too-many-statements -def password_reset_confirm_wrapper(request, uidb36=None, token=None): +class PasswordResetConfirmWrapper(PasswordResetConfirmView): """ - A wrapper around django.contrib.auth.views.password_reset_confirm. - Needed because we want to set the user as active at this step. - We also optionally do some additional password policy checks. + A wrapper around django.contrib.auth.views.PasswordResetConfirmView. + Needed because we want to set the user as active at this step. + We also optionally do some additional password policy checks. """ - # convert old-style base36-encoded user id to base64 - uidb64 = _uidb36_to_uidb64(uidb36) - platform_name = { - "platform_name": configuration_helpers.get_value('platform_name', settings.PLATFORM_NAME) - } - # User can not get this link unless account recovery feature is enabled. - if 'is_account_recovery' in request.GET and not is_secondary_email_feature_enabled(): - raise Http404 + reset_url_token = 'set-password' - try: - uid_int = base36_to_int(uidb36) - if request.user.is_authenticated and request.user.id != uid_int: - raise Http404 + def __init__(self): + self.post_reset_login = False + self.platform_name = PasswordResetConfirmWrapper._get_platform_name() + self.validlink = False + self.user = None + self.uidb36 = '' + self.token = '' + self.uidb64 = '' + self.uid_int = -1 - user = User.objects.get(id=uid_int) - except (ValueError, User.DoesNotExist): - # if there's any error getting a user, just let django's - # password_reset_confirm function handle it. - return password_reset_confirm( - request, uidb64=uidb64, token=token, extra_context=platform_name - ) + def _process_password_reset_success(self, request, token, uidb64, extra_context): + self.user = self.get_user(uidb64) + form = SetPasswordForm(self.user, request.POST) + if self.token_generator.check_token(self.user, token) and form.is_valid(): + self.form_valid(form) + url = reverse('password_reset_complete') + return HttpResponseRedirect(url) + else: + context = self.get_context_data() + if extra_context is not None: + context.update(extra_context) + return self.render_to_response(context) + + def _set_token_in_session(self, request, token): + """ + method to store password reset token in session received in reset password url + """ + if not token: + return + session = request.session + session[INTERNAL_RESET_SESSION_TOKEN] = token + session.save() + + @staticmethod + def _get_platform_name(): + return {"platform_name": configuration_helpers.get_value('platform_name', settings.PLATFORM_NAME)} + + def _set_user(self, request): + try: + self.uid_int = base36_to_int(self.uidb36) + if request.user.is_authenticated and request.user.id != self.uid_int: + raise Http404 + self.user = User.objects.get(id=self.uid_int) + except (ValueError, User.DoesNotExist): + # if there's any error getting a user, just let django's + # password_reset_confirm function handle it. + return super(PasswordResetConfirmWrapper, self).dispatch(request, uidb64=self.uidb64, token=self.token, + extra_context=self.platform_name) + + def _handle_retired_user(self, request): + """ + method responsible to stop password reset in case user is retired + """ - if UserRetirementRequest.has_user_requested_retirement(user): - # Refuse to reset the password of any user that has requested retirement. context = { 'validlink': True, 'form': None, 'title': _('Password reset unsuccessful'), 'err_msg': _('Error in resetting your password.'), } - context.update(platform_name) + context.update(self.platform_name) return TemplateResponse( request, 'registration/password_reset_confirm.html', context ) - if waffle().is_enabled(PREVENT_AUTH_USER_WRITES): + def _handle_system_unavailability(self, request): + """ + method to stop password reset process if system is under maintenance + """ + context = { 'validlink': False, 'form': None, 'title': _('Password reset unsuccessful'), 'err_msg': SYSTEM_MAINTENANCE_MSG, } - context.update(platform_name) + context.update(self.platform_name) return TemplateResponse( request, 'registration/password_reset_confirm.html', context ) - if request.method == 'POST': + def _validate_password(self, password, request): + try: + validate_password(password, user=self.user) + except ValidationError as err: + context = { + 'validlink': True, + 'form': None, + 'title': _('Password reset unsuccessful'), + 'err_msg': ' '.join(err.messages), + } + context.update(self.platform_name) + return TemplateResponse( + request, 'registration/password_reset_confirm.html', context + ) + + def _handle_password_reset_failure(self, response): + form_valid = response.context_data['form'].is_valid() if response.context_data['form'] else False + if not form_valid: + log.warning( + u'Unable to reset password for user [%s] because form is not valid. ' + u'A possible cause is that the user had an invalid reset token', + self.user.username, + ) + response.context_data['err_msg'] = _('Error in resetting your password. Please try again.') + return response + + def _handle_primary_email_update(self, updated_user): + try: + updated_user.email = updated_user.account_recovery.secondary_email + updated_user.account_recovery.delete() + # emit an event that the user changed their secondary email to the primary email + tracker.emit( + SETTING_CHANGE_INITIATED, + { + "setting": "email", + "old": self.user.email, + "new": updated_user.email, + "user_id": updated_user.id, + } + ) + except ObjectDoesNotExist: + log.error('Account recovery process initiated without AccountRecovery instance for user {username}' + .format(username=updated_user.username)) + + def _handle_password_creation(self, request, updated_user): + messages.success( + request, + HTML(_( + u'{html_start}Password Creation Complete{html_end}' + u'Your password has been created. {bold_start}{email}{bold_end} is now your primary login email.' + )).format( + support_url=configuration_helpers.get_value('SUPPORT_SITE_LINK', settings.SUPPORT_SITE_LINK), + html_start=HTML('

'), + html_end=HTML('

'), + bold_start=HTML(''), + bold_end=HTML(''), + email=updated_user.email, + ), + extra_tags='account-recovery aa-icon submission-success' + ) + + def post(self, request, *args, **kwargs): # We have to make a copy of request.POST because it is a QueryDict object which is immutable until copied. # We have to use request.POST because the password_reset_confirm method takes in the request and a user's # password is set to the request.POST['new_password1'] field. We have to also normalize the new_password2 @@ -366,104 +463,63 @@ def password_reset_confirm_wrapper(request, uidb36=None, token=None): request.POST['new_password2'] = normalize_password(request.POST['new_password2']) password = request.POST['new_password1'] - - try: - validate_password(password, user=user) - except ValidationError as err: - # We have a password reset attempt which violates some security - # policy, or any other validation. Use the existing Django template to communicate that - # back to the user. - context = { - 'validlink': True, - 'form': None, - 'title': _('Password reset unsuccessful'), - 'err_msg': ' '.join(err.messages), - } - context.update(platform_name) - return TemplateResponse( - request, 'registration/password_reset_confirm.html', context - ) + response = self._validate_password(password, request) + if response: + return response if 'is_account_recovery' in request.GET: - response = password_reset_confirm( - request, - uidb64=uidb64, - token=token, - extra_context=platform_name, - template_name='registration/password_reset_confirm.html', - post_reset_redirect='signin_user', - ) + self.post_reset_login = True + response = self._process_password_reset_success(request, self.token, self.uidb64, + extra_context=self.platform_name) else: - response = password_reset_confirm( - request, uidb64=uidb64, token=token, extra_context=platform_name - ) + response = self._process_password_reset_success(request, self.token, self.uidb64, + extra_context=self.platform_name) # If password reset was unsuccessful a template response is returned (status_code 200). # Check if form is invalid then show an error to the user. # Note if password reset was successful we get response redirect (status_code 302). if response.status_code == 200: - form_valid = response.context_data['form'].is_valid() if response.context_data['form'] else False - if not form_valid: - log.warning( - u'Unable to reset password for user [%s] because form is not valid. ' - u'A possible cause is that the user had an invalid reset token', - user.username, - ) - response.context_data['err_msg'] = _('Error in resetting your password. Please try again.') - return response + return self._handle_password_reset_failure(response) - # get the updated user - updated_user = User.objects.get(id=uid_int) + updated_user = User.objects.get(id=self.uid_int) if 'is_account_recovery' in request.GET: - try: - updated_user.email = updated_user.account_recovery.secondary_email - updated_user.account_recovery.delete() - # emit an event that the user changed their secondary email to the primary email - tracker.emit( - SETTING_CHANGE_INITIATED, - { - "setting": "email", - "old": user.email, - "new": updated_user.email, - "user_id": updated_user.id, - } - ) - except ObjectDoesNotExist: - log.error( - u'Account recovery process initiated without AccountRecovery instance for user {username}'.format( - username=updated_user.username - ) - ) + self._handle_primary_email_update(updated_user) updated_user.save() - if response.status_code == 302 and 'is_account_recovery' in request.GET: - messages.success( - request, - HTML(_( - u'{html_start}Password Creation Complete{html_end}' - u'Your password has been created. {bold_start}{email}{bold_end} is now your primary login email.' - )).format( - support_url=configuration_helpers.get_value('SUPPORT_SITE_LINK', settings.SUPPORT_SITE_LINK), - html_start=HTML('

'), - html_end=HTML('

'), - bold_start=HTML(''), - bold_end=HTML(''), - email=updated_user.email, - ), - extra_tags='account-recovery aa-icon submission-success' - ) - else: - response = password_reset_confirm( - request, uidb64=uidb64, token=token, extra_context=platform_name - ) + self._handle_password_creation(request, updated_user) - response_was_successful = response.context_data.get('validlink') - if response_was_successful and not user.is_active: - user.is_active = True - user.save() + return response - return response + def dispatch(self, *args, **kwargs): + self.uidb36 = kwargs.get('uidb36') + self.token = kwargs.get('token') + self.uidb64 = _uidb36_to_uidb64(self.uidb36) + + # User can not get this link unless account recovery feature is enabled. + if 'is_account_recovery' in self.request.GET and not is_secondary_email_feature_enabled(): + raise Http404 + + response = self._set_user(self.request) + if response: + return response + if UserRetirementRequest.has_user_requested_retirement(self.user): + return self._handle_retired_user(self.request) + if waffle().is_enabled(PREVENT_AUTH_USER_WRITES): + return self._handle_system_unavailability(self.request) + + if self.request.method == 'POST': + return self.post(self.request, *args, **kwargs) + else: + self._set_token_in_session(self.request, self.token) + token = self.reset_url_token + response = super(PasswordResetConfirmWrapper, self).dispatch(self.request, uidb64=self.uidb64, token=token, + extra_context=self.platform_name) + response_was_successful = response.context_data.get('validlink') + if response_was_successful and not self.user.is_active: + self.user.is_active = True + self.user.save() + return response def _get_user_from_email(email): diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_reset_password.py b/openedx/core/djangoapps/user_authn/views/tests/test_reset_password.py index dd6ae185c1..220e0df0e8 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_reset_password.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_reset_password.py @@ -13,6 +13,8 @@ from django.conf import settings from django.contrib.auth.hashers import UNUSABLE_PASSWORD_PREFIX, make_password from django.contrib.auth.models import AnonymousUser, User from django.contrib.auth.tokens import default_token_generator +from django.contrib.auth.views import INTERNAL_RESET_SESSION_TOKEN, PasswordResetConfirmView +from django.contrib.sessions.middleware import SessionMiddleware from django.core import mail from django.core.cache import cache from django.http import Http404 @@ -34,8 +36,8 @@ from openedx.core.djangoapps.user_api.models import UserRetirementRequest from openedx.core.djangoapps.user_api.tests.test_views import UserAPITestCase from openedx.core.djangoapps.user_api.accounts import EMAIL_MAX_LENGTH, EMAIL_MIN_LENGTH from openedx.core.djangoapps.user_authn.views.password_reset import ( - SETTING_CHANGE_INITIATED, password_reset, password_reset_confirm_wrapper -) + SETTING_CHANGE_INITIATED, password_reset, + PasswordResetConfirmWrapper) from openedx.core.djangolib.testing.utils import CacheIsolationTestCase from student.tests.factories import UserFactory from student.tests.test_configuration_overrides import fake_get_value @@ -45,6 +47,12 @@ from util.password_policy_validators import create_validator_config from util.testing import EventTestMixin +def process_request(request): + middleware = SessionMiddleware() + middleware.process_request(request) + request.session.save() + + @unittest.skipUnless( settings.ROOT_URLCONF == "lms.urls", "reset password tests should only run in LMS" @@ -55,7 +63,6 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): Tests that clicking reset password sends email, and doesn't activate the user """ request_factory = RequestFactory() - ENABLED_CACHES = ['default'] def setUp(self): # pylint: disable=arguments-differ @@ -303,8 +310,9 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): kwargs={"uidb36": uidb36, "token": token} ) ) + process_request(bad_request) bad_request.user = AnonymousUser() - password_reset_confirm_wrapper(bad_request, uidb36, token) + PasswordResetConfirmWrapper.as_view()(bad_request, uidb36=uidb36, token=token) self.user = User.objects.get(pk=self.user.pk) self.assertFalse(self.user.is_active) @@ -317,8 +325,9 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): kwargs={"uidb36": self.uidb36, "token": self.token} ) good_reset_req = self.request_factory.get(url) + process_request(good_reset_req) good_reset_req.user = self.user - password_reset_confirm_wrapper(good_reset_req, self.uidb36, self.token) + PasswordResetConfirmWrapper.as_view()(good_reset_req, uidb36=self.uidb36, token=self.token) self.user = User.objects.get(pk=self.user.pk) self.assertTrue(self.user.is_active) @@ -331,8 +340,9 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): kwargs={"uidb36": self.uidb36, "token": self.token} ) good_reset_req = self.request_factory.get(url) + process_request(good_reset_req) good_reset_req.user = AnonymousUser() - password_reset_confirm_wrapper(good_reset_req, self.uidb36, self.token) + PasswordResetConfirmWrapper.as_view()(good_reset_req, uidb36=self.uidb36, token=self.token) self.user = User.objects.get(pk=self.user.pk) self.assertTrue(self.user.is_active) @@ -348,10 +358,11 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): ) request_params = {'new_password1': 'password1', 'new_password2': 'password2'} confirm_request = self.request_factory.post(url, data=request_params) + process_request(confirm_request) confirm_request.user = self.user # Make a password reset request with mismatching passwords. - resp = password_reset_confirm_wrapper(confirm_request, self.uidb36, self.token) + resp = PasswordResetConfirmWrapper.as_view()(confirm_request, uidb36=self.uidb36, token=self.token) # Verify the response status code is: 200 with password reset fail and also verify that # the user is not marked as active. @@ -373,7 +384,7 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): ) reset_req = self.request_factory.get(url) reset_req.user = self.user - resp = password_reset_confirm_wrapper(reset_req, self.uidb36, self.token) + resp = PasswordResetConfirmWrapper.as_view()(reset_req, uidb36=self.uidb36, token=self.token) # Verify the response status code is: 200 with password reset fail and also verify that # the user is not marked as active. @@ -388,7 +399,7 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): ) for request in [self.request_factory.get(url), self.request_factory.post(url)]: request.user = self.user - response = password_reset_confirm_wrapper(request, self.uidb36, self.token) + response = PasswordResetConfirmWrapper.as_view()(request, uidb36=self.uidb36, token=self.token) assert response.context_data['err_msg'] == SYSTEM_MAINTENANCE_MSG self.user.refresh_from_db() assert not self.user.is_active @@ -408,8 +419,10 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): password = u'p\u212bssword' request_params = {'new_password1': password, 'new_password2': password} confirm_request = self.request_factory.post(url, data=request_params) + process_request(confirm_request) + confirm_request.session[INTERNAL_RESET_SESSION_TOKEN] = self.token confirm_request.user = self.user - __ = password_reset_confirm_wrapper(confirm_request, self.uidb36, self.token) + __ = PasswordResetConfirmWrapper.as_view()(confirm_request, uidb36=self.uidb36, token=self.token) user = User.objects.get(pk=self.user.pk) salt_val = user.password.split('$')[1] @@ -445,11 +458,11 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): confirm_request.user = self.user # Make a password reset request with minimum/maximum passwords characters. - response = password_reset_confirm_wrapper(confirm_request, self.uidb36, self.token) + response = PasswordResetConfirmWrapper.as_view()(confirm_request, uidb36=self.uidb36, token=self.token) self.assertEqual(response.context_data['err_msg'], password_dict['error_message']) - @patch('openedx.core.djangoapps.user_authn.views.password_reset.password_reset_confirm') + @patch.object(PasswordResetConfirmView, 'dispatch') @patch("openedx.core.djangoapps.site_configuration.helpers.get_value", fake_get_value) def test_reset_password_good_token_configuration_override(self, reset_confirm): """ @@ -460,8 +473,9 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): kwargs={"uidb36": self.uidb36, "token": self.token} ) good_reset_req = self.request_factory.get(url) + process_request(good_reset_req) good_reset_req.user = self.user - password_reset_confirm_wrapper(good_reset_req, self.uidb36, self.token) + PasswordResetConfirmWrapper.as_view()(good_reset_req, uidb36=self.uidb36, token=self.token) confirm_kwargs = reset_confirm.call_args[1] self.assertEqual(confirm_kwargs['extra_context']['platform_name'], 'Fake University') self.user = User.objects.get(pk=self.user.pk) @@ -497,7 +511,8 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): reset_request = self.request_factory.get(reset_url) reset_request.user = UserFactory.create() - self.assertRaises(Http404, password_reset_confirm_wrapper, reset_request, self.uidb36, self.token) + self.assertRaises(Http404, PasswordResetConfirmWrapper.as_view(), reset_request, uidb36=self.uidb36, + token=self.token) @ddt.ddt