diff --git a/common/djangoapps/student/forms.py b/common/djangoapps/student/forms.py index 84bdb17e0c..93adc2107d 100644 --- a/common/djangoapps/student/forms.py +++ b/common/djangoapps/student/forms.py @@ -19,11 +19,7 @@ from django.template import loader from django.conf import settings from microsite_configuration import microsite from student.models import CourseEnrollmentAllowed -from util.password_policy_validators import ( - validate_password_length, - validate_password_complexity, - validate_password_dictionary, -) +from util.password_policy_validators import validate_password_strength from openedx.core.djangoapps.theming import helpers as theming_helpers @@ -223,9 +219,7 @@ class AccountCreationForm(forms.Form): raise ValidationError(_("Username and password fields cannot match")) if self.enforce_password_policy: try: - validate_password_length(password) - validate_password_complexity(password) - validate_password_dictionary(password) + validate_password_strength(password) except ValidationError, err: raise ValidationError(_("Password: ") + "; ".join(err.messages)) return password diff --git a/common/djangoapps/student/tests/test_password_policy.py b/common/djangoapps/student/tests/test_password_policy.py index c7c7d8a30f..2c8fe1b6dd 100644 --- a/common/djangoapps/student/tests/test_password_policy.py +++ b/common/djangoapps/student/tests/test_password_policy.py @@ -59,7 +59,7 @@ class TestPasswordPolicy(TestCase): obj = json.loads(response.content) self.assertEqual( obj['value'], - "Password: Invalid Length (must be 12 characters or less)", + "Password: Invalid Length (must be 12 characters or fewer)", ) @patch.dict("django.conf.settings.PASSWORD_COMPLEXITY", {'UPPER': 3}) diff --git a/common/djangoapps/student/tests/test_reset_password.py b/common/djangoapps/student/tests/test_reset_password.py index 6ca006765b..927b281c57 100644 --- a/common/djangoapps/student/tests/test_reset_password.py +++ b/common/djangoapps/student/tests/test_reset_password.py @@ -6,8 +6,8 @@ import re import unittest from django.core.cache import cache +from django.core.urlresolvers import reverse from django.conf import settings -from django.test import TestCase from django.test.client import RequestFactory from django.contrib.auth.models import User from django.contrib.auth.hashers import UNUSABLE_PASSWORD_PREFIX @@ -29,6 +29,10 @@ from .test_microsite import fake_microsite_get_value from openedx.core.djangoapps.theming import helpers as theming_helpers +@unittest.skipUnless( + settings.ROOT_URLCONF == "lms.urls", + "reset password tests should only run in LMS" +) @ddt.ddt class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): """ Tests that clicking reset password sends email, and doesn't activate the user @@ -50,10 +54,6 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): self.user_bad_passwd.password = UNUSABLE_PASSWORD_PREFIX self.user_bad_passwd.save() - def uidb36_to_uidb64(self, uidb36=None): - """ Converts uidb36 into uidb64 """ - return force_text(urlsafe_base64_encode(force_bytes(base36_to_int(uidb36 or self.uidb36)))) - @patch('student.views.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) def test_user_bad_password_reset(self): """Tests password reset behavior for user with password marked UNUSABLE_PASSWORD_PREFIX""" @@ -219,29 +219,37 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): ) self.assertEqual(from_addr, "no-reply@fakeuniversity.com") - @patch('student.views.password_reset_confirm') - def test_reset_password_bad_token(self, reset_confirm): + @ddt.data( + ('invalidUid', 'invalid_token'), + (None, 'invalid_token'), + ('invalidUid', None), + ) + @ddt.unpack + def test_reset_password_bad_token(self, uidb36, token): """Tests bad token and uidb36 in password reset""" + if uidb36 is None: + uidb36 = self.uidb36 + if token is None: + token = self.token - bad_reset_req = self.request_factory.get('/password_reset_confirm/NO-OP/') - password_reset_confirm_wrapper(bad_reset_req, 'NO', 'OP') - confirm_kwargs = reset_confirm.call_args[1] - - self.assertEquals(confirm_kwargs['uidb64'], self.uidb36_to_uidb64('NO')) - - self.assertEquals(confirm_kwargs['token'], 'OP') + bad_request = self.request_factory.get( + reverse( + "password_reset_confirm", + kwargs={"uidb36": uidb36, "token": token} + ) + ) + password_reset_confirm_wrapper(bad_request, uidb36, token) self.user = User.objects.get(pk=self.user.pk) self.assertFalse(self.user.is_active) - @patch('student.views.password_reset_confirm') - def test_reset_password_good_token(self, reset_confirm): + def test_reset_password_good_token(self): """Tests good token and uidb36 in password reset""" - - good_reset_req = self.request_factory.get('/password_reset_confirm/{0}-{1}/'.format(self.uidb36, self.token)) + url = reverse( + "password_reset_confirm", + kwargs={"uidb36": self.uidb36, "token": self.token} + ) + good_reset_req = self.request_factory.get(url) password_reset_confirm_wrapper(good_reset_req, self.uidb36, self.token) - confirm_kwargs = reset_confirm.call_args[1] - self.assertEquals(confirm_kwargs['uidb64'], self.uidb36_to_uidb64()) - self.assertEquals(confirm_kwargs['token'], self.token) self.user = User.objects.get(pk=self.user.pk) self.assertTrue(self.user.is_active) @@ -249,20 +257,13 @@ class ResetPasswordTests(EventTestMixin, CacheIsolationTestCase): @patch("microsite_configuration.microsite.get_value", fake_microsite_get_value) def test_reset_password_good_token_microsite(self, reset_confirm): """Tests password reset confirmation page for micro site""" - - good_reset_req = self.request_factory.get('/password_reset_confirm/{0}-{1}/'.format(self.uidb36, self.token)) + url = reverse( + "password_reset_confirm", + kwargs={"uidb36": self.uidb36, "token": self.token} + ) + good_reset_req = self.request_factory.get(url) 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') - - @patch('student.views.password_reset_confirm') - def test_reset_password_with_reused_password(self, reset_confirm): - """Tests good token and uidb36 in password reset""" - - good_reset_req = self.request_factory.get('/password_reset_confirm/{0}-{1}/'.format(self.uidb36, self.token)) - password_reset_confirm_wrapper(good_reset_req, self.uidb36, self.token) - confirm_kwargs = reset_confirm.call_args[1] - self.assertEquals(confirm_kwargs['uidb64'], self.uidb36_to_uidb64()) - self.assertEquals(confirm_kwargs['token'], self.token) self.user = User.objects.get(pk=self.user.pk) self.assertTrue(self.user.is_active) diff --git a/common/djangoapps/student/views.py b/common/djangoapps/student/views.py index 86d32dfc83..b3754dd1ca 100644 --- a/common/djangoapps/student/views.py +++ b/common/djangoapps/student/views.py @@ -99,11 +99,7 @@ from util.milestones_helpers import ( ) from microsite_configuration import microsite -from util.password_policy_validators import ( - validate_password_length, validate_password_complexity, - validate_password_dictionary -) - +from util.password_policy_validators import validate_password_strength import third_party_auth from third_party_auth import pipeline, provider from student.helpers import ( @@ -2125,105 +2121,140 @@ def password_reset(request): }) -def password_reset_confirm_wrapper( - request, - uidb36=None, - token=None, -): - """ A wrapper around django.contrib.auth.views.password_reset_confirm. - Needed because we want to set the user as active at this step. +def uidb36_to_uidb64(uidb36): """ - # cribbed from django.contrib.auth.views.password_reset_confirm + Needed to support old password reset URLs that use base36-encoded user IDs + https://github.com/django/django/commit/1184d077893ff1bc947e45b00a4d565f3df81776#diff-c571286052438b2e3190f8db8331a92bR231 + Args: + uidb36: base36-encoded user ID + + Returns: base64-encoded user ID. Otherwise returns a dummy, invalid ID + """ + try: + uidb64 = force_text(urlsafe_base64_encode(force_bytes(base36_to_int(uidb36)))) + except ValueError: + uidb64 = '1' # dummy invalid ID (incorrect padding for base64) + return uidb64 + + +def validate_password(user, password): + """ + Tie in password policy enforcement as an optional level of + security protection + + Args: + user: the user object whose password we're checking. + password: the user's proposed new password. + + Returns: + is_valid_password: a boolean indicating if the new password + passes the validation. + err_msg: an error message if there's a violation of one of the password + checks. Otherwise, `None`. + """ + err_msg = None + + if settings.FEATURES.get('ENFORCE_PASSWORD_POLICY', False): + try: + validate_password_strength(password) + except ValidationError as err: + err_msg = _('Password: ') + '; '.join(err.messages) + + # also, check the password reuse policy + if not PasswordHistory.is_allowable_password_reuse(user, password): + if user.is_staff: + num_distinct = settings.ADVANCED_SECURITY_CONFIG['MIN_DIFFERENT_STAFF_PASSWORDS_BEFORE_REUSE'] + else: + num_distinct = settings.ADVANCED_SECURITY_CONFIG['MIN_DIFFERENT_STUDENT_PASSWORDS_BEFORE_REUSE'] + # Because of how ngettext is, splitting the following into shorter lines would be ugly. + # pylint: disable=line-too-long + err_msg = ungettext( + "You are re-using a password that you have used recently. You must have {num} distinct password before reusing a previous password.", + "You are re-using a password that you have used recently. You must have {num} distinct passwords before reusing a previous password.", + num_distinct + ).format(num=num_distinct) + + # also, check to see if passwords are getting reset too frequent + if PasswordHistory.is_password_reset_too_soon(user): + num_days = settings.ADVANCED_SECURITY_CONFIG['MIN_TIME_IN_DAYS_BETWEEN_ALLOWED_RESETS'] + # Because of how ngettext is, splitting the following into shorter lines would be ugly. + # pylint: disable=line-too-long + err_msg = ungettext( + "You are resetting passwords too frequently. Due to security policies, {num} day must elapse between password resets.", + "You are resetting passwords too frequently. Due to security policies, {num} days must elapse between password resets.", + num_days + ).format(num=num_days) + + is_password_valid = err_msg is None + + return is_password_valid, err_msg + + +def password_reset_confirm_wrapper(request, uidb36=None, token=None): + """ + 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. + """ + # convert old-style base36-encoded user id to base64 + uidb64 = uidb36_to_uidb64(uidb36) + platform_name = { + "platform_name": microsite.get_value('platform_name', settings.PLATFORM_NAME) + } try: uid_int = base36_to_int(uidb36) user = User.objects.get(id=uid_int) - user.is_active = True - user.save() except (ValueError, User.DoesNotExist): - pass - - # tie in password strength enforcement as an optional level of - # security protection - err_msg = None + # 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 + ) if request.method == 'POST': password = request.POST['new_password1'] - if settings.FEATURES.get('ENFORCE_PASSWORD_POLICY', False): - try: - validate_password_length(password) - validate_password_complexity(password) - validate_password_dictionary(password) - except ValidationError, err: - err_msg = _('Password: ') + '; '.join(err.messages) + is_password_valid, password_err_msg = validate_password(user, password) + if not is_password_valid: + # We have a password reset attempt which violates some security + # policy. Use the existing Django template to communicate that + # back to the user. + context = { + 'validlink': False, + 'form': None, + 'title': _('Password reset unsuccessful'), + 'err_msg': password_err_msg, + } + context.update(platform_name) + return TemplateResponse( + request, 'registration/password_reset_confirm.html', context + ) - # also, check the password reuse policy - if not PasswordHistory.is_allowable_password_reuse(user, password): - if user.is_staff: - num_distinct = settings.ADVANCED_SECURITY_CONFIG['MIN_DIFFERENT_STAFF_PASSWORDS_BEFORE_REUSE'] - else: - num_distinct = settings.ADVANCED_SECURITY_CONFIG['MIN_DIFFERENT_STUDENT_PASSWORDS_BEFORE_REUSE'] - # Because of how ngettext is, splitting the following into shorter lines would be ugly. - # pylint: disable=line-too-long - err_msg = ungettext( - "You are re-using a password that you have used recently. You must have {num} distinct password before reusing a previous password.", - "You are re-using a password that you have used recently. You must have {num} distinct passwords before reusing a previous password.", - num_distinct - ).format(num=num_distinct) + # remember what the old password hash is before we call down + old_password_hash = user.password - # also, check to see if passwords are getting reset too frequent - if PasswordHistory.is_password_reset_too_soon(user): - num_days = settings.ADVANCED_SECURITY_CONFIG['MIN_TIME_IN_DAYS_BETWEEN_ALLOWED_RESETS'] - # Because of how ngettext is, splitting the following into shorter lines would be ugly. - # pylint: disable=line-too-long - err_msg = ungettext( - "You are resetting passwords too frequently. Due to security policies, {num} day must elapse between password resets.", - "You are resetting passwords too frequently. Due to security policies, {num} days must elapse between password resets.", - num_days - ).format(num=num_days) + response = password_reset_confirm( + request, uidb64=uidb64, token=token, extra_context=platform_name + ) + + # get the updated user + updated_user = User.objects.get(id=uid_int) + + # did the password hash change, if so record it in the PasswordHistory + if updated_user.password != old_password_hash: + entry = PasswordHistory() + entry.create(updated_user) - if err_msg: - # We have an password reset attempt which violates some security policy, use the - # existing Django template to communicate this back to the user - context = { - 'validlink': True, - 'form': None, - 'title': _('Password reset unsuccessful'), - 'err_msg': err_msg, - 'platform_name': microsite.get_value('platform_name', settings.PLATFORM_NAME), - } - return TemplateResponse(request, 'registration/password_reset_confirm.html', context) else: - # we also want to pass settings.PLATFORM_NAME in as extra_context - extra_context = {"platform_name": microsite.get_value('platform_name', settings.PLATFORM_NAME)} + response = password_reset_confirm( + request, uidb64=uidb64, token=token, extra_context=platform_name + ) - # Support old password reset URLs that used base36 encoded user IDs. - # https://github.com/django/django/commit/1184d077893ff1bc947e45b00a4d565f3df81776#diff-c571286052438b2e3190f8db8331a92bR231 - try: - uidb64 = force_text(urlsafe_base64_encode(force_bytes(base36_to_int(uidb36)))) - except ValueError: - uidb64 = '1' # dummy invalid ID (incorrect padding for base64) + response_was_successful = response.context_data.get('validlink') + if response_was_successful and not user.is_active: + user.is_active = True + user.save() - if request.method == 'POST': - # remember what the old password hash is before we call down - old_password_hash = user.password - - result = password_reset_confirm( - request, uidb64=uidb64, token=token, extra_context=extra_context - ) - - # get the updated user - updated_user = User.objects.get(id=uid_int) - - # did the password hash change, if so record it in the PasswordHistory - if updated_user.password != old_password_hash: - entry = PasswordHistory() - entry.create(updated_user) - - return result - else: - return password_reset_confirm( - request, uidb64=uidb64, token=token, extra_context=extra_context - ) + return response def reactivation_email_for_user(user): diff --git a/common/djangoapps/util/password_policy_validators.py b/common/djangoapps/util/password_policy_validators.py index f8132832c5..a268e635e8 100644 --- a/common/djangoapps/util/password_policy_validators.py +++ b/common/djangoapps/util/password_policy_validators.py @@ -15,6 +15,26 @@ from django.conf import settings import nltk +def validate_password_strength(value): + """ + This function loops through each validator defined in this file + and applies it to a user's proposed password + + Args: + value: a user's proposed password + + Returns: None, but raises a ValidationError if the proposed password + fails any one of the validators in password_validators + """ + password_validators = [ + validate_password_length, + validate_password_complexity, + validate_password_dictionary, + ] + for validator in password_validators: + validator(value) + + def validate_password_length(value): """ Validator that enforces minimum length of a password @@ -28,7 +48,7 @@ def validate_password_length(value): if min_length and len(value) < min_length: raise ValidationError(message.format(_("must be {0} characters or more").format(min_length)), code=code) elif max_length and len(value) > max_length: - raise ValidationError(message.format(_("must be {0} characters or less").format(max_length)), code=code) + raise ValidationError(message.format(_("must be {0} characters or fewer").format(max_length)), code=code) def validate_password_complexity(value): diff --git a/lms/djangoapps/courseware/tests/test_password_history.py b/lms/djangoapps/courseware/tests/test_password_history.py index 6408fa90ed..61ec39d3ce 100644 --- a/lms/djangoapps/courseware/tests/test_password_history.py +++ b/lms/djangoapps/courseware/tests/test_password_history.py @@ -71,6 +71,18 @@ class TestPasswordHistory(LoginEnrollmentTestCase): history = PasswordHistory() history.create(user) + def assertPasswordResetError(self, response, error_message): + """ + This method is a custom assertion that verifies that a password reset + view returns an error response as expected. + Args: + response: response from calling a password reset endpoint + error_message: message we expect to see in the response + + """ + self.assertFalse(response.context_data['validlink']) + self.assertIn(error_message, response.content) + @patch.dict("django.conf.settings.ADVANCED_SECURITY_CONFIG", {'MIN_DAYS_FOR_STAFF_ACCOUNTS_PASSWORD_RESETS': None}) @patch.dict("django.conf.settings.ADVANCED_SECURITY_CONFIG", {'MIN_DAYS_FOR_STUDENT_ACCOUNTS_PASSWORD_RESETS': None}) def test_no_forced_password_change(self): @@ -168,10 +180,7 @@ class TestPasswordHistory(LoginEnrollmentTestCase): 'new_password2': 'foo' }, follow=True) - self.assertIn( - err_msg, - resp.content - ) + self.assertPasswordResetError(resp, err_msg) # now retry with a different password resp = self.client.post('/password_reset_confirm/{0}-{1}/'.format(uidb36, token), { @@ -179,10 +188,7 @@ class TestPasswordHistory(LoginEnrollmentTestCase): 'new_password2': 'bar' }, follow=True) - self.assertIn( - success_msg, - resp.content - ) + self.assertIn(success_msg, resp.content) @patch.dict("django.conf.settings.ADVANCED_SECURITY_CONFIG", {'MIN_DIFFERENT_STAFF_PASSWORDS_BEFORE_REUSE': 2}) def test_staff_password_reset_reuse(self): @@ -204,10 +210,7 @@ class TestPasswordHistory(LoginEnrollmentTestCase): 'new_password2': 'foo', }, follow=True) - self.assertIn( - err_msg, - resp.content - ) + self.assertPasswordResetError(resp, err_msg) # now use different one user = User.objects.get(email=staff_email) @@ -219,10 +222,7 @@ class TestPasswordHistory(LoginEnrollmentTestCase): 'new_password2': 'bar', }, follow=True) - self.assertIn( - success_msg, - resp.content - ) + self.assertIn(success_msg, resp.content) # now try again with the first one user = User.objects.get(email=staff_email) @@ -234,11 +234,7 @@ class TestPasswordHistory(LoginEnrollmentTestCase): 'new_password2': 'foo', }, follow=True) - # should be rejected - self.assertIn( - err_msg, - resp.content - ) + self.assertPasswordResetError(resp, err_msg) # now use different one user = User.objects.get(email=staff_email) @@ -250,10 +246,7 @@ class TestPasswordHistory(LoginEnrollmentTestCase): 'new_password2': 'baz', }, follow=True) - self.assertIn( - success_msg, - resp.content - ) + self.assertIn(success_msg, resp.content) # now we should be able to reuse the first one user = User.objects.get(email=staff_email) @@ -265,10 +258,7 @@ class TestPasswordHistory(LoginEnrollmentTestCase): 'new_password2': 'foo', }, follow=True) - self.assertIn( - success_msg, - resp.content - ) + self.assertIn(success_msg, resp.content) @patch.dict("django.conf.settings.ADVANCED_SECURITY_CONFIG", {'MIN_TIME_IN_DAYS_BETWEEN_ALLOWED_RESETS': 1}) def test_password_reset_frequency_limit(self): @@ -308,10 +298,7 @@ class TestPasswordHistory(LoginEnrollmentTestCase): 'new_password2': 'foo', }, follow=True) - self.assertIn( - success_msg, - resp.content - ) + self.assertIn(success_msg, resp.content) @patch.dict("django.conf.settings.FEATURES", {'ENFORCE_PASSWORD_POLICY': True}) @override_settings(PASSWORD_MIN_LENGTH=6) @@ -350,7 +337,4 @@ class TestPasswordHistory(LoginEnrollmentTestCase): 'new_password2': 'foofoo', }, follow=True) - self.assertIn( - success_msg, - resp.content - ) + self.assertIn(success_msg, resp.content)