diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 60beb09d06..f838d317e2 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -215,6 +215,19 @@ def is_username_retired(username): return User.objects.filter(username__in=list(locally_hashed_usernames)).exists() +def is_email_retired(email): + """ + Checks to see if the given email has been previously retired + """ + locally_hashed_emails = user_util.get_all_retired_emails( + email, + settings.RETIRED_USER_SALTS, + settings.RETIRED_EMAIL_FMT + ) + + return User.objects.filter(email__in=list(locally_hashed_emails)).exists() + + def get_retired_username_by_username(username): """ If a UserRetirementStatus object with an original_username matching the given username exists, diff --git a/common/djangoapps/student/signals/receivers.py b/common/djangoapps/student/signals/receivers.py index 4c0b9b6bb4..1d222386a8 100644 --- a/common/djangoapps/student/signals/receivers.py +++ b/common/djangoapps/student/signals/receivers.py @@ -11,7 +11,10 @@ from student.helpers import ( AccountValidationError, USERNAME_EXISTS_MSG_FMT ) -from student.models import is_username_retired +from student.models import ( + is_email_retired, + is_username_retired, +) def update_last_login(sender, user, **kwargs): # pylint: disable=unused-argument @@ -46,3 +49,10 @@ def on_user_updated(sender, instance, **kwargs): # pylint: disable=unused-argum USERNAME_EXISTS_MSG_FMT.format(username=instance.username), field="username" ) + + # Check for a retired email. + if is_email_retired(instance.email): + raise AccountValidationError( + EMAIL_EXISTS_MSG_FMT.format(username=instance.email), + field="email" + ) diff --git a/common/djangoapps/student/tests/test_email.py b/common/djangoapps/student/tests/test_email.py index d2d6bba0e8..fdff0ea4af 100644 --- a/common/djangoapps/student/tests/test_email.py +++ b/common/djangoapps/student/tests/test_email.py @@ -18,7 +18,12 @@ from openedx.core.djangoapps.site_configuration import helpers as configuration_ from openedx.core.djangoapps.theming.tests.test_util import with_comprehensive_theme from openedx.core.djangoapps.user_api.config.waffle import PREVENT_AUTH_USER_WRITES, SYSTEM_MAINTENANCE_MSG, waffle from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, CacheIsolationMixin -from student.models import PendingEmailChange, Registration, UserProfile +from student.models import ( + PendingEmailChange, + Registration, + UserProfile, + get_retired_email_by_email +) from student.tests.factories import PendingEmailChangeFactory, RegistrationFactory, UserFactory from student.views import ( SETTING_CHANGE_INITIATED, @@ -325,6 +330,16 @@ class EmailChangeRequestTests(EventTestMixin, CacheIsolationTestCase): UserFactory.create(email=self.new_email) self.assertEqual(self.do_email_validation(self.new_email), 'An account with this e-mail already exists.') + def test_retired_email(self): + """ + Assert the expected error message from the email validation method for an email address + that corresponds with an already-retired account. + """ + user = UserFactory.create(email=self.new_email) + user.email = get_retired_email_by_email(self.new_email) + user.save() + self.assertEqual(self.do_email_validation(self.new_email), 'An account with this e-mail already exists.') + @patch('django.core.mail.send_mail') @patch('student.views.management.render_to_string', Mock(side_effect=mock_render_to_string, autospec=True)) def test_email_failure(self, send_mail): diff --git a/common/djangoapps/student/tests/test_retirement.py b/common/djangoapps/student/tests/test_retirement.py index 48c592f6ac..3f5e28e597 100644 --- a/common/djangoapps/student/tests/test_retirement.py +++ b/common/djangoapps/student/tests/test_retirement.py @@ -17,7 +17,8 @@ from student.models import ( get_potentially_retired_user_by_username_and_hash, get_retired_email_by_email, get_retired_username_by_username, - is_username_retired + is_username_retired, + is_email_retired ) from student.tests.factories import UserFactory @@ -132,6 +133,29 @@ def test_is_username_retired_not_retired(): assert not is_username_retired(user.username) +def test_is_email_retired_is_retired(): + """ + Check functionality of is_email_retired when email is retired + """ + user = UserFactory() + original_email = user.email + retired_email = get_retired_email_by_email(user.email) + + # Fake email retirement. + user.email = retired_email + user.save() + + assert is_email_retired(original_email) + + +def test_is_email_retired_not_retired(): + """ + Check functionality of is_email_retired when email is not retired + """ + user = UserFactory() + assert not is_email_retired(user.email) + + def test_get_retired_email(): """ Basic testing of retired emails. diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 825e59ebfc..7ed4554a87 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -86,7 +86,9 @@ from student.models import ( CourseEnrollment, CourseEnrollmentAllowed, ManualEnrollmentAudit, - NonExistentCourseError + NonExistentCourseError, + get_retired_email_by_email, + get_retired_username_by_username ) from student.roles import CourseBetaTesterRole, CourseFinanceAdminRole, CourseInstructorRole, CourseSalesAdminRole from student.tests.factories import AdminFactory, UserFactory @@ -842,6 +844,28 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas self.assertEqual(manual_enrollments.count(), 1) self.assertTrue(manual_enrollments[0].state_transition, UNENROLLED_TO_ENROLLED) + def test_user_with_retired_email_in_csv(self): + + # prep a retired user + user = UserFactory.create(username='old_test_student', email='test_student@example.com') + user.email = get_retired_email_by_email(user.email) + user.username = get_retired_username_by_username(user.username) + user.is_active = False + user.save() + + csv_content = "test_student@example.com,test_student,tester,USA" \ + + uploaded_file = SimpleUploadedFile("temp.csv", csv_content) + response = self.client.post(self.url, {'students_list': uploaded_file}) + self.assertEqual(response.status_code, 200) + data = json.loads(response.content) + self.assertNotEquals(len(data['row_errors']), 0) + self.assertEquals( + data['row_errors'][0]['response'], + 'Invalid email {email}.'.format(email='test_student@example.com') + ) + self.assertFalse(User.objects.filter(email='test_student@example.com').exists()) + def test_user_with_already_existing_username_in_csv(self): """ If the username already exists (but not the email), @@ -907,8 +931,16 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas def test_users_created_and_enrolled_successfully_if_others_fail(self): + # prep a retired user + user = UserFactory.create(username='old_test_student_4', email='test_student4@example.com') + user.email = get_retired_email_by_email(user.email) + user.username = get_retired_username_by_username(user.username) + user.is_active = False + user.save() + csv_content = "test_student1@example.com,test_student_1,tester1,USA\n" \ "test_student3@example.com,test_student_1,tester3,CA\n" \ + "test_student4@example.com,test_student_4,tester4,USA\n" \ "test_student2@example.com,test_student_2,tester2,USA" uploaded_file = SimpleUploadedFile("temp.csv", csv_content) @@ -916,10 +948,18 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas self.assertEqual(response.status_code, 200) data = json.loads(response.content) self.assertNotEquals(len(data['row_errors']), 0) - self.assertEquals(data['row_errors'][0]['response'], 'Username {user} already exists.'.format(user='test_student_1')) + self.assertEquals( + data['row_errors'][0]['response'], + 'Username {user} already exists.'.format(user='test_student_1') + ) + self.assertEquals( + data['row_errors'][1]['response'], + 'Invalid email {email}.'.format(email='test_student4@example.com') + ) self.assertTrue(User.objects.filter(username='test_student_1', email='test_student1@example.com').exists()) self.assertTrue(User.objects.filter(username='test_student_2', email='test_student2@example.com').exists()) self.assertFalse(User.objects.filter(email='test_student3@example.com').exists()) + self.assertFalse(User.objects.filter(email='test_student4@example.com').exists()) manual_enrollments = ManualEnrollmentAudit.objects.all() self.assertEqual(manual_enrollments.count(), 2) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 329195a0f1..19849087ba 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -16,7 +16,6 @@ import StringIO import time import unicodecsv -from django.apps import apps from django.conf import settings from django.contrib.auth.models import User from django.core.exceptions import ObjectDoesNotExist, PermissionDenied, ValidationError @@ -106,7 +105,8 @@ from student.models import ( UserProfile, anonymous_id_for_user, get_user_by_username_or_email, - unique_id_for_user + unique_id_for_user, + is_email_retired ) from student.roles import CourseFinanceAdminRole, CourseSalesAdminRole from submissions import api as sub_api # installed from the edx-submissions repository @@ -377,7 +377,6 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man row_errors.append({ 'username': username, 'email': email, 'response': _('Invalid email {email_address}.').format(email_address=email)}) else: - UserRetirementStatus = apps.get_model('user_api', 'UserRetirementStatus') if User.objects.filter(email=email).exists(): # Email address already exists. assume it is the correct user # and just register the user in the course and send an enrollment email. @@ -414,20 +413,16 @@ def register_and_enroll_students(request, course_id): # pylint: disable=too-man state_transition=UNENROLLED_TO_ENROLLED, ) enroll_email(course_id=course_id, student_email=email, auto_enroll=True, email_students=True, email_params=email_params) - elif (UserRetirementStatus.objects - .select_related('current_state') - .filter(original_email=email) - .exclude(current_state__state_name='COMPLETE') - .exists()): - # Somebody is attempting to enroll a user who has initiated retirement but still exists in - # general. Simply block these attempts. - general_errors.append({ - 'username': '', - 'email': '', - 'response': _('Invalid email {email_address}.').format(email_address=email), - }) - log.warning(u'Email address %s is associated with a user which has initiated account retirement, ' + - u'so course enrollment was blocked.', email) + elif is_email_retired(email): + # We are either attempting to enroll a retired user or create a new user with an email which is + # already associated with a retired account. Simply block these attempts. + row_errors.append({ + 'username': username, + 'email': email, + 'response': _('Invalid email {email_address}.').format(email_address=email), + }) + log.warning(u'Email address %s is associated with a retired user, so course enrollment was ' + + u'blocked.', email) else: # This email does not yet exist, so we need to create a new account # If username already exists in the database, then create_and_enroll_user diff --git a/openedx/core/djangoapps/user_api/accounts/utils.py b/openedx/core/djangoapps/user_api/accounts/utils.py index 2c0e08d219..94d9e1d9d9 100644 --- a/openedx/core/djangoapps/user_api/accounts/utils.py +++ b/openedx/core/djangoapps/user_api/accounts/utils.py @@ -6,7 +6,6 @@ import re import string from urlparse import urlparse -from django.apps import apps from django.conf import settings from django.contrib.auth.models import User from django.utils.translation import ugettext as _ @@ -16,6 +15,7 @@ from completion import waffle as completion_waffle from completion.models import BlockCompletion from openedx.core.djangoapps.site_configuration.models import SiteConfiguration from openedx.core.djangoapps.theming.helpers import get_config_value_from_site_or_settings, get_current_site +from student.models import is_email_retired from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError @@ -197,25 +197,6 @@ def generate_password(length=12, chars=string.letters + string.digits): def email_exists(email): """ - Check an email against the User and UserRetirementStatus models for - existence. + Check an email against the User model for existence. """ - exists = False - # Normal case, check users in the auth_user table. - if User.objects.filter(email=email).exists(): - exists = True - else: - # Handle case where another user with the same email address has - # initiated retirement (account deletion), but they are still in - # the retirement queue in any state which is not COMPLETE. - UserRetirementStatus = apps.get_model('user_api', 'UserRetirementStatus') - try: - if (UserRetirementStatus.objects - .select_related('current_state') - .filter(original_email=email) - .exclude(current_state__state_name='COMPLETE') - .exists()): - exists = True - except UserRetirementStatus.DoesNotExist: - pass - return exists + return User.objects.filter(email=email).exists() or is_email_retired(email)