authenticate user after their reset confirmation goes through

make password validation code more DRY

grammar nit: "less" -> "fewer"

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

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})

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"""
-
-        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')
+        if uidb36 is None:
+            uidb36 = self.uidb36
+        if token is None:
+            token = self.token
+
+        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)

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):
+    """
+    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
     """
-    # cribbed from django.contrib.auth.views.password_reset_confirm
+    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)
-
-        # 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)
-
-    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)}
+        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
+            )
 
-        # 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)
+        # remember what the old password hash is before we call down
+        old_password_hash = user.password
 
-        if request.method == 'POST':
-            # remember what the old password hash is before we call down
-            old_password_hash = user.password
+        response = password_reset_confirm(
+            request, uidb64=uidb64, token=token, extra_context=platform_name
+        )
 
-            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)
 
-            # 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)
+
+    else:
+        response = password_reset_confirm(
+            request, uidb64=uidb64, token=token, extra_context=platform_name
+        )
 
-            # 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)
+        response_was_successful = response.context_data.get('validlink')
+        if response_was_successful and not user.is_active:
+            user.is_active = True
+            user.save()
 
-            return result
-        else:
-            return password_reset_confirm(
-                request, uidb64=uidb64, token=token, extra_context=extra_context
-            )
+    return response
 
 
 def reactivation_email_for_user(user):

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):

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)