diff --git a/common/djangoapps/student/forms.py b/common/djangoapps/student/forms.py index 89aa4204b3..2958aaa2a4 100644 --- a/common/djangoapps/student/forms.py +++ b/common/djangoapps/student/forms.py @@ -8,8 +8,6 @@ from importlib import import_module from django import forms from django.conf import settings -from django.contrib.auth.forms import PasswordResetForm -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.core.exceptions import ValidationError @@ -28,9 +26,8 @@ from openedx.core.djangoapps.theming.helpers import get_current_site from openedx.core.djangoapps.user_api import accounts as accounts_settings from openedx.core.djangoapps.user_api.accounts.utils import is_secondary_email_feature_enabled from openedx.core.djangoapps.user_api.preferences.api import get_user_preference -from openedx.core.djangoapps.user_authn.views.password_reset import send_password_reset_email_for_user from student.message_types import AccountRecovery as AccountRecoveryMessage -from student.models import AccountRecovery, CourseEnrollmentAllowed, email_exists_or_retired +from student.models import CourseEnrollmentAllowed, email_exists_or_retired from util.password_policy_validators import validate_password @@ -67,56 +64,6 @@ def send_account_recovery_email_for_user(user, request, email=None): ace.send(msg) -class PasswordResetFormNoActive(PasswordResetForm): - error_messages = { - 'unknown': _("That e-mail address doesn't have an associated " - "user account. Are you sure you've registered?"), - 'unusable': _("The user account associated with this e-mail " - "address cannot reset the password."), - } - - is_account_recovery = True - - def clean_email(self): - """ - This is a literal copy from Django 1.4.5's django.contrib.auth.forms.PasswordResetForm - Except removing the requirement of active users - 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 - self.users_cache = User.objects.filter(email__iexact=email) - - if len(self.users_cache) == 0 and is_secondary_email_feature_enabled(): - # Check if user has entered the secondary email. - self.users_cache = User.objects.filter( - id__in=AccountRecovery.objects.filter(secondary_email__iexact=email, is_active=True).values_list('user') - ) - self.is_account_recovery = not bool(self.users_cache) - - if not len(self.users_cache): - raise forms.ValidationError(self.error_messages['unknown']) - if any((user.password.startswith(UNUSABLE_PASSWORD_PREFIX)) - for user in self.users_cache): - raise forms.ValidationError(self.error_messages['unusable']) - return email - - def save(self, # pylint: disable=arguments-differ - use_https=False, - token_generator=default_token_generator, - request=None, - **_kwargs): - """ - Generates a one-use only link for resetting password and sends to the - user. - """ - for user in self.users_cache: - if self.is_account_recovery: - send_password_reset_email_for_user(user, request) - else: - send_account_recovery_email_for_user(user, request, user.account_recovery.secondary_email) - - class TrueCheckbox(widgets.CheckboxInput): """ A checkbox widget that only accepts "true" (case-insensitive) as true. diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 1da1390752..eb2f339d33 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -65,7 +65,7 @@ from openedx.core.djangoapps.user_api.models import UserRetirementRequest from openedx.core.djangoapps.user_api.preferences import api as preferences_api from openedx.core.djangoapps.user_authn.message_types import PasswordReset from openedx.core.djangolib.markup import HTML, Text -from student.forms import AccountCreationForm, PasswordResetFormNoActive +from student.forms import AccountCreationForm from student.helpers import DISABLE_UNENROLL_CERT_STATES, cert_info, generate_activation_email_context from student.message_types import EmailChange, EmailChangeConfirmation, RecoveryEmailCreate from student.models import ( @@ -676,7 +676,7 @@ def password_change_request_handler(request): if email: try: - from openedx.core.djangoapps.user_api.accounts.api import request_password_change + from openedx.core.djangoapps.user_authn.views.password_reset import request_password_change request_password_change(email, request.is_secure()) user = user if user.is_authenticated else get_user_from_email(email=email) destroy_oauth_tokens(user) @@ -746,6 +746,7 @@ def password_reset(request): AUDIT_LOG.warning("Rate limit exceeded in password_reset") return HttpResponseForbidden() + from openedx.core.djangoapps.user_authn.views.password_reset import PasswordResetFormNoActive form = PasswordResetFormNoActive(request.POST) if form.is_valid(): form.save(use_https=request.is_secure(), diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index c58fa0f0d2..7385a72a80 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -20,7 +20,7 @@ from six import text_type # pylint: disable=ungrouped-imports from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.theming.helpers import get_current_request -from openedx.core.djangoapps.user_api import accounts, errors, forms, helpers +from openedx.core.djangoapps.user_api import accounts, errors, helpers from openedx.core.djangoapps.user_api.config.waffle import PREVENT_AUTH_USER_WRITES, SYSTEM_MAINTENANCE_MSG, waffle from openedx.core.djangoapps.user_api.errors import ( AccountUpdateError, @@ -360,44 +360,6 @@ def activate_account(activation_key): registration.activate() -@helpers.intercept_errors(errors.UserAPIInternalError, ignore_errors=[errors.UserAPIRequestError]) -def request_password_change(email, is_secure): - """Email a single-use link for performing a password reset. - - Users must confirm the password change before we update their information. - - Args: - email (str): An email address - orig_host (str): An originating host, extracted from a request with get_host - is_secure (bool): Whether the request was made with HTTPS - - Returns: - None - - Raises: - errors.UserNotFound - AccountRequestError - errors.UserAPIInternalError: the operation failed due to an unexpected error. - - """ - # Binding data to a form requires that the data be passed as a dictionary - # to the Form class constructor. - form = forms.PasswordResetFormNoActive({'email': email}) - - # Validate that a user exists with the given email address. - if form.is_valid(): - # Generate a single-use link for performing a password reset - # and email it to the user. - form.save( - from_email=configuration_helpers.get_value('email_from_address', settings.DEFAULT_FROM_EMAIL), - use_https=is_secure, - request=get_current_request(), - ) - else: - # No user with the provided email address exists. - raise errors.UserNotFound - - def get_name_validation_error(name): """Get the built-in validation error message for when the user's real name is invalid in some way (we wonder how). diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py index d9e048bd42..eeb45b0372 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_api.py @@ -30,7 +30,6 @@ from openedx.core.djangoapps.user_api.accounts import PRIVATE_VISIBILITY, USERNA from openedx.core.djangoapps.user_api.accounts.api import ( activate_account, get_account_settings, - request_password_change, update_account_settings ) from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import ( # pylint: disable=unused-import @@ -580,57 +579,6 @@ class AccountActivationAndPasswordChangeTest(CreateAccountMixin, TestCase): with waffle().override(PREVENT_AUTH_USER_WRITES, True): activate_account(activation_key) - @skip_unless_lms - def test_request_password_change(self): - # Create and activate an account - self.create_account(self.USERNAME, self.PASSWORD, self.EMAIL) - self.assertEqual(len(mail.outbox), 1) - - user = User.objects.get(username=self.USERNAME) - activation_key = self.get_activation_key(user) - activate_account(activation_key) - - request = RequestFactory().post('/password') - request.user = Mock() - request.site = SiteFactory() - - with patch('crum.get_current_request', return_value=request): - # Request a password change - request_password_change(self.EMAIL, self.IS_SECURE) - - # Verify that a new email message has been sent - self.assertEqual(len(mail.outbox), 2) - - # Verify that the body of the message contains something that looks - # like an activation link - email_body = mail.outbox[0].body - result = re.search(r'(?Phttps?://[^\s]+)', email_body) - self.assertIsNot(result, None) - - @skip_unless_lms - def test_request_password_change_invalid_user(self): - with self.assertRaises(UserNotFound): - request_password_change(self.EMAIL, self.IS_SECURE) - - # Verify that no email messages have been sent - self.assertEqual(len(mail.outbox), 0) - - @skip_unless_lms - def test_request_password_change_inactive_user(self): - # Create an account, but do not activate it - self.create_account(self.USERNAME, self.PASSWORD, self.EMAIL) - self.assertEqual(len(mail.outbox), 1) - - request = RequestFactory().post('/password') - request.user = Mock() - request.site = SiteFactory() - - with patch('crum.get_current_request', return_value=request): - request_password_change(self.EMAIL, self.IS_SECURE) - - # Verify that the activation email was still sent - self.assertEqual(len(mail.outbox), 2) - def _assert_is_datetime(self, timestamp): """ Internal helper to validate the type of the provided timestamp diff --git a/openedx/core/djangoapps/user_api/forms.py b/openedx/core/djangoapps/user_api/forms.py deleted file mode 100644 index 5375cc5a57..0000000000 --- a/openedx/core/djangoapps/user_api/forms.py +++ /dev/null @@ -1,8 +0,0 @@ -# pylint: disable=unused-import -# TODO: eventually move this implementation into the user_api -""" -Django Administration forms module -""" -from __future__ import absolute_import - -from student.forms import PasswordResetFormNoActive diff --git a/openedx/core/djangoapps/user_authn/views/password_reset.py b/openedx/core/djangoapps/user_authn/views/password_reset.py index 8178478693..663bd3a363 100644 --- a/openedx/core/djangoapps/user_authn/views/password_reset.py +++ b/openedx/core/djangoapps/user_authn/views/password_reset.py @@ -1,6 +1,12 @@ """ Handles password resets logic. """ +from django import forms + +from django.contrib.auth.forms import PasswordResetForm + from django.conf import settings +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.http import HttpResponse from django.utils.decorators import method_decorator @@ -16,12 +22,16 @@ from rest_framework.views import APIView from openedx.core.djangoapps.ace_common.template_context import get_base_template_context from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers -from openedx.core.djangoapps.theming.helpers import get_current_site -from openedx.core.djangoapps.user_api import accounts +from openedx.core.djangoapps.theming.helpers import get_current_request, get_current_site +from openedx.core.djangoapps.user_api import accounts, errors, helpers +from openedx.core.djangoapps.user_api.accounts.utils import is_secondary_email_feature_enabled from openedx.core.djangoapps.user_api.helpers import FormDescription from openedx.core.djangoapps.user_api.preferences.api import get_user_preference from openedx.core.djangoapps.user_authn.message_types import PasswordReset +from student.models import AccountRecovery +from student.forms import send_account_recovery_email_for_user + def get_password_reset_form(): """Return a description of the password reset form. @@ -69,18 +79,6 @@ def get_password_reset_form(): return form_desc -class PasswordResetView(APIView): - """HTTP end-point for GETting a description of the password reset form. """ - - # This end-point is available to anonymous users, - # so do not require authentication. - authentication_classes = [] - - @method_decorator(ensure_csrf_cookie) - def get(self, request): - return HttpResponse(get_password_reset_form().to_json(), content_type="application/json") - - def send_password_reset_email_for_user(user, request, preferred_email=None): """ Send out a password reset email for the given user. @@ -112,3 +110,108 @@ def send_password_reset_email_for_user(user, request, preferred_email=None): user_context=message_context, ) ace.send(msg) + + +class PasswordResetFormNoActive(PasswordResetForm): + """ + A modified version of the default Django password reset form to handle + unknown or unusable email addresses without leaking data. + """ + error_messages = { + 'unknown': _("That e-mail address doesn't have an associated " + "user account. Are you sure you've registered?"), + 'unusable': _("The user account associated with this e-mail " + "address cannot reset the password."), + } + + is_account_recovery = True + users_cache = [] + + def clean_email(self): + """ + This is a literal copy from Django 1.4.5's django.contrib.auth.forms.PasswordResetForm + Except removing the requirement of active users + 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 + self.users_cache = User.objects.filter(email__iexact=email) + + if not self.users_cache and is_secondary_email_feature_enabled(): + # Check if user has entered the secondary email. + self.users_cache = User.objects.filter( + id__in=AccountRecovery.objects.filter(secondary_email__iexact=email, is_active=True).values_list('user') + ) + self.is_account_recovery = not bool(self.users_cache) + + if not self.users_cache: + raise forms.ValidationError(self.error_messages['unknown']) + if any((user.password.startswith(UNUSABLE_PASSWORD_PREFIX)) + for user in self.users_cache): + raise forms.ValidationError(self.error_messages['unusable']) + return email + + def save(self, # pylint: disable=arguments-differ + use_https=False, + token_generator=default_token_generator, + request=None, + **_kwargs): + """ + Generates a one-use only link for resetting password and sends to the + user. + """ + for user in self.users_cache: + if self.is_account_recovery: + send_password_reset_email_for_user(user, request) + else: + send_account_recovery_email_for_user(user, request, user.account_recovery.secondary_email) + + +class PasswordResetView(APIView): + """HTTP end-point for GETting a description of the password reset form. """ + + # This end-point is available to anonymous users, + # so do not require authentication. + authentication_classes = [] + + @method_decorator(ensure_csrf_cookie) + def get(self, request): + return HttpResponse(get_password_reset_form().to_json(), content_type="application/json") + + +@helpers.intercept_errors(errors.UserAPIInternalError, ignore_errors=[errors.UserAPIRequestError]) +def request_password_change(email, is_secure): + """Email a single-use link for performing a password reset. + + Users must confirm the password change before we update their information. + + Args: + email (str): An email address + orig_host (str): An originating host, extracted from a request with get_host + is_secure (bool): Whether the request was made with HTTPS + + Returns: + None + + Raises: + errors.UserNotFound + AccountRequestError + errors.UserAPIInternalError: the operation failed due to an unexpected error. + + """ + # Binding data to a form requires that the data be passed as a dictionary + # to the Form class constructor. + form = PasswordResetFormNoActive({'email': email}) + + # Validate that a user exists with the given email address. + if form.is_valid(): + # Generate a single-use link for performing a password reset + # and email it to the user. + form.save( + from_email=configuration_helpers.get_value('email_from_address', settings.DEFAULT_FROM_EMAIL), + use_https=is_secure, + request=get_current_request(), + ) + else: + # No user with the provided email address exists. + raise errors.UserNotFound diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_password.py b/openedx/core/djangoapps/user_authn/views/tests/test_password.py new file mode 100644 index 0000000000..538e9c209d --- /dev/null +++ b/openedx/core/djangoapps/user_authn/views/tests/test_password.py @@ -0,0 +1,89 @@ +# -*- coding: utf-8 -*- +""" +Tests for user authorization password-related functionality. +""" +import re +from mock import Mock, patch + +from django.core import mail +from django.contrib.auth.models import User +from django.test import TestCase +from django.test.client import RequestFactory + +from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory +from openedx.core.djangoapps.user_api.accounts.tests.test_api import CreateAccountMixin +from openedx.core.djangoapps.user_api.accounts.api import ( + activate_account, +) +from openedx.core.djangoapps.user_api.errors import UserNotFound +from openedx.core.djangoapps.user_authn.views.password_reset import request_password_change +from openedx.core.djangolib.testing.utils import skip_unless_lms + +from student.models import Registration + + +class TestRequestPasswordChange(CreateAccountMixin, TestCase): + """ + Tests for users who request a password change. + """ + + USERNAME = u'claire-underwood' + PASSWORD = u'ṕáśśẃőŕd' + EMAIL = u'claire+underwood@example.com' + + IS_SECURE = False + + def get_activation_key(self, user): + registration = Registration.objects.get(user=user) + return registration.activation_key + + @skip_unless_lms + def test_request_password_change(self): + # Create and activate an account + self.create_account(self.USERNAME, self.PASSWORD, self.EMAIL) + self.assertEqual(len(mail.outbox), 1) + + user = User.objects.get(username=self.USERNAME) + activation_key = self.get_activation_key(user) + activate_account(activation_key) + + request = RequestFactory().post('/password') + request.user = Mock() + request.site = SiteFactory() + + with patch('crum.get_current_request', return_value=request): + # Request a password change + request_password_change(self.EMAIL, self.IS_SECURE) + + # Verify that a new email message has been sent + self.assertEqual(len(mail.outbox), 2) + + # Verify that the body of the message contains something that looks + # like an activation link + email_body = mail.outbox[0].body + result = re.search(r'(?Phttps?://[^\s]+)', email_body) + self.assertIsNot(result, None) + + @skip_unless_lms + def test_request_password_change_invalid_user(self): + with self.assertRaises(UserNotFound): + request_password_change(self.EMAIL, self.IS_SECURE) + + # Verify that no email messages have been sent + self.assertEqual(len(mail.outbox), 0) + + @skip_unless_lms + def test_request_password_change_inactive_user(self): + # Create an account, but do not activate it + self.create_account(self.USERNAME, self.PASSWORD, self.EMAIL) + self.assertEqual(len(mail.outbox), 1) + + request = RequestFactory().post('/password') + request.user = Mock() + request.site = SiteFactory() + + with patch('crum.get_current_request', return_value=request): + request_password_change(self.EMAIL, self.IS_SECURE) + + # Verify that the activation email was still sent + self.assertEqual(len(mail.outbox), 2) diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_views.py b/openedx/core/djangoapps/user_authn/views/tests/test_views.py index 455e3a9f39..11303c9637 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_views.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_views.py @@ -167,7 +167,7 @@ class UserAccountUpdateTest(CacheIsolationTestCase, UrlResetMixin): assert response_dict['success'] def test_password_change_failure(self): - with mock.patch('openedx.core.djangoapps.user_api.accounts.api.request_password_change', + with mock.patch('openedx.core.djangoapps.user_authn.views.password_reset.request_password_change', side_effect=UserAPIInternalError): self._change_password() self.assertRaises(UserAPIInternalError)