From 9f80fd2b873e644d5e00f893e715e47ef8cd7ac1 Mon Sep 17 00:00:00 2001 From: Waheed Ahmed Date: Thu, 29 Aug 2019 18:10:23 +0500 Subject: [PATCH] Fixed password reset for authenticated user. Password reset link for one user is working for other logged in users. Fixed by verifying the token with request.user if authenticated. LEARNER-5114 --- .../student/tests/test_reset_password.py | 40 ++++++++++++++++++- common/djangoapps/student/views/management.py | 10 +++-- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/common/djangoapps/student/tests/test_reset_password.py b/common/djangoapps/student/tests/test_reset_password.py index 28e2e64175..0ca7b6f21c 100644 --- a/common/djangoapps/student/tests/test_reset_password.py +++ b/common/djangoapps/student/tests/test_reset_password.py @@ -11,10 +11,11 @@ import unittest import ddt from django.conf import settings from django.contrib.auth.hashers import UNUSABLE_PASSWORD_PREFIX, make_password -from django.contrib.auth.models import User +from django.contrib.auth.models import AnonymousUser, User from django.contrib.auth.tokens import default_token_generator from django.core import mail from django.core.cache import cache +from django.http import Http404 from django.test.client import RequestFactory from django.test.utils import override_settings from django.urls import reverse @@ -286,6 +287,7 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): kwargs={"uidb36": uidb36, "token": token} ) ) + bad_request.user = AnonymousUser() password_reset_confirm_wrapper(bad_request, uidb36, token) self.user = User.objects.get(pk=self.user.pk) self.assertFalse(self.user.is_active) @@ -299,6 +301,21 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): kwargs={"uidb36": self.uidb36, "token": self.token} ) good_reset_req = self.request_factory.get(url) + good_reset_req.user = self.user + password_reset_confirm_wrapper(good_reset_req, self.uidb36, self.token) + self.user = User.objects.get(pk=self.user.pk) + self.assertTrue(self.user.is_active) + + def test_reset_password_good_token_with_anonymous_user(self): + """ + Tests good token and uidb36 in password reset for anonymous user + """ + url = reverse( + "password_reset_confirm", + kwargs={"uidb36": self.uidb36, "token": self.token} + ) + good_reset_req = self.request_factory.get(url) + good_reset_req.user = AnonymousUser() password_reset_confirm_wrapper(good_reset_req, self.uidb36, self.token) self.user = User.objects.get(pk=self.user.pk) self.assertTrue(self.user.is_active) @@ -315,6 +332,7 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): ) request_params = {'new_password1': 'password1', 'new_password2': 'password2'} confirm_request = self.request_factory.post(url, data=request_params) + confirm_request.user = self.user # Make a password reset request with mismatching passwords. resp = password_reset_confirm_wrapper(confirm_request, self.uidb36, self.token) @@ -338,6 +356,7 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): kwargs={'uidb36': self.uidb36, 'token': self.token} ) reset_req = self.request_factory.get(url) + reset_req.user = self.user resp = password_reset_confirm_wrapper(reset_req, self.uidb36, self.token) # Verify the response status code is: 200 with password reset fail and also verify that @@ -352,6 +371,7 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): kwargs={"uidb36": self.uidb36, "token": self.token} ) 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) assert response.context_data['err_msg'] == SYSTEM_MAINTENANCE_MSG self.user.refresh_from_db() @@ -371,7 +391,8 @@ 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) - response = password_reset_confirm_wrapper(confirm_request, self.uidb36, self.token) + confirm_request.user = self.user + __ = password_reset_confirm_wrapper(confirm_request, self.uidb36, self.token) user = User.objects.get(pk=self.user.pk) salt_val = user.password.split('$')[1] @@ -404,6 +425,7 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): ) request_params = {'new_password1': password_dict['password'], 'new_password2': password_dict['password']} confirm_request = self.request_factory.post(url, data=request_params) + 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) @@ -421,6 +443,7 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): kwargs={"uidb36": self.uidb36, "token": self.token} ) good_reset_req = self.request_factory.get(url) + good_reset_req.user = self.user password_reset_confirm_wrapper(good_reset_req, self.uidb36, self.token) confirm_kwargs = reset_confirm.call_args[1] self.assertEquals(confirm_kwargs['extra_context']['platform_name'], 'Fake University') @@ -445,3 +468,16 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): subj, _, _, _ = send_email.call_args[0] self.assertIn(platform_name, subj) + + def test_reset_password_with_other_user_link(self): + """ + Tests that user should not be able to reset password through other user's token + """ + reset_url = reverse( + "password_reset_confirm", + kwargs={"uidb36": self.uidb36, "token": self.token} + ) + 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) diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 3bfc0c0391..692ade4a28 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -137,8 +137,8 @@ def index(request, extra_context=None, user=AnonymousUser()): courses = get_courses(user) if configuration_helpers.get_value( - "ENABLE_COURSE_SORTING_BY_START_DATE", - settings.FEATURES["ENABLE_COURSE_SORTING_BY_START_DATE"], + "ENABLE_COURSE_SORTING_BY_START_DATE", + settings.FEATURES["ENABLE_COURSE_SORTING_BY_START_DATE"], ): courses = sort_by_start_date(courses) else: @@ -685,7 +685,6 @@ def password_change_request_handler(request): # no user associated with the email if configuration_helpers.get_value('ENABLE_PASSWORD_RESET_FAILURE_EMAIL', settings.FEATURES['ENABLE_PASSWORD_RESET_FAILURE_EMAIL']): - site = get_current_site() message_context = get_base_template_context(site) @@ -809,6 +808,9 @@ def password_reset_confirm_wrapper(request, uidb36=None, token=None): try: uid_int = base36_to_int(uidb36) + if request.user.is_authenticated and request.user.id != uid_int: + raise Http404 + user = User.objects.get(id=uid_int) except (ValueError, User.DoesNotExist): # if there's any error getting a user, just let django's @@ -1167,7 +1169,7 @@ def confirm_email_change(request, key): # pylint: disable=unused-argument # Send it to the old email... try: ace.send(msg) - except Exception: # pylint: disable=broad-except + except Exception: # pylint: disable=broad-except log.warning('Unable to send confirmation email to old address', exc_info=True) response = render_to_response("email_change_failed.html", {'email': user.email}) transaction.set_rollback(True)