diff --git a/common/djangoapps/student/forms.py b/common/djangoapps/student/forms.py index 68a6618667..c844dc6899 100644 --- a/common/djangoapps/student/forms.py +++ b/common/djangoapps/student/forms.py @@ -19,8 +19,7 @@ from django.core.validators import RegexValidator, slug_re from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_api import accounts as accounts_settings -from openedx.core.djangoapps.user_api.accounts.utils import email_exists -from student.models import CourseEnrollmentAllowed +from student.models import CourseEnrollmentAllowed, email_exists_or_retired from util.password_policy_validators import password_max_length, password_min_length, validate_password @@ -295,7 +294,7 @@ class AccountCreationForm(forms.Form): # reject the registration. if not CourseEnrollmentAllowed.objects.filter(email=email).exists(): raise ValidationError(_("Unauthorized email address.")) - if email_exists(email): + if email_exists_or_retired(email): raise ValidationError( _( "It looks like {email} belongs to an existing account. Try again with a different email address." diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index e299a5bfb5..c0a610765a 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -48,7 +48,8 @@ from student.models import ( Registration, UserAttribute, UserProfile, - unique_id_for_user + unique_id_for_user, + email_exists_or_retired ) @@ -652,7 +653,7 @@ def do_create_account(form, custom_form=None): USERNAME_EXISTS_MSG_FMT.format(username=proposed_username), field="username" ) - elif User.objects.filter(email=user.email): + elif email_exists_or_retired(user.email): raise AccountValidationError( _("An account with the Email '{email}' already exists.").format(email=user.email), field="email" diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index f838d317e2..da7ce8d4b1 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -228,6 +228,13 @@ def is_email_retired(email): return User.objects.filter(email__in=list(locally_hashed_emails)).exists() +def email_exists_or_retired(email): + """ + Check an email against the User model for existence. + """ + return User.objects.filter(email=email).exists() or is_email_retired(email) + + def get_retired_username_by_username(username): """ If a UserRetirementStatus object with an original_username matching the given username exists, @@ -2246,18 +2253,30 @@ class CourseAccessRole(models.Model): #### Helper methods for use from python manage.py shell and other classes. +def strip_if_string(value): + if isinstance(value, six.string_types): + return value.strip() + return value + + def get_user_by_username_or_email(username_or_email): """ Return a User object, looking up by email if username_or_email contains a '@', otherwise by username. Raises: - User.DoesNotExist is lookup fails. + User.DoesNotExist if no user object can be found, the user was + retired, or the user is in the process of being retired. """ + username_or_email = strip_if_string(username_or_email) if '@' in username_or_email: - return User.objects.get(email=username_or_email) + user = User.objects.get(email=username_or_email) else: - return User.objects.get(username=username_or_email) + user = User.objects.get(username=username_or_email) + UserRetirementRequest = apps.get_model('user_api', 'UserRetirementRequest') + if UserRetirementRequest.has_user_requested_retirement(user): + raise User.DoesNotExist + return user def get_user(email): diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 1d899007ee..98199bab1a 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -95,7 +95,7 @@ from student.models import ( UserSignupSource, UserStanding, create_comments_service_user, - username_or_email_exists_or_retired, + email_exists_or_retired, ) from student.signals import REFUND_ORDER from student.tasks import send_activation_email @@ -1252,7 +1252,7 @@ def validate_new_email(user, new_email): if new_email == user.email: raise ValueError(_('Old email is the same as the new email.')) - if username_or_email_exists_or_retired(new_email): + if email_exists_or_retired(new_email): raise ValueError(_('An account with this e-mail already exists.')) diff --git a/lms/djangoapps/ccx/api/v0/views.py b/lms/djangoapps/ccx/api/v0/views.py index a80c1f1cde..5cb6513645 100644 --- a/lms/djangoapps/ccx/api/v0/views.py +++ b/lms/djangoapps/ccx/api/v0/views.py @@ -430,6 +430,12 @@ class CCXListView(GenericAPIView): ) try: + # Retired users should effectively appear to not exist when + # attempts are made to modify them, so a direct User model email + # lookup is sufficient here. This corner case relies on the fact + # that we scramble emails immediately during user lock-out. Of + # course, the normal cases are that the email just never existed, + # or it is currently associated with an active account. coach = User.objects.get(email=valid_input['coach_email']) except User.DoesNotExist: return Response( diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index f8aab91939..6e8977e90c 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -27,7 +27,12 @@ from lms.djangoapps.grades.signals.signals import PROBLEM_RAW_SCORE_CHANGED from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_api.models import UserPreference -from student.models import CourseEnrollment, CourseEnrollmentAllowed, anonymous_id_for_user +from student.models import ( + CourseEnrollment, + CourseEnrollmentAllowed, + anonymous_id_for_user, + is_email_retired, +) from submissions import api as sub_api # installed from the edx-submissions repository from submissions.models import score_set from track.event_transaction_utils import ( @@ -44,6 +49,9 @@ log = logging.getLogger(__name__) class EmailEnrollmentState(object): """ Store the complete enrollment state of an email in a class """ def __init__(self, course_id, email): + # N.B. retired users are not a concern here because they should be + # handled at a higher level (i.e. in enroll_email). Besides, this + # class creates readonly objects. exists_user = User.objects.filter(email=email).exists() if exists_user: user = User.objects.get(email=email) @@ -142,7 +150,7 @@ def enroll_email(course_id, student_email, auto_enroll=False, email_students=Fal email_params['email_address'] = student_email email_params['full_name'] = previous_state.full_name send_mail_to_student(student_email, email_params, language=language) - else: + elif not is_email_retired(student_email): cea, _ = CourseEnrollmentAllowed.objects.get_or_create(course_id=course_id, email=student_email) cea.auto_enroll = auto_enroll cea.save() diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 7ed4554a87..8704181323 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -845,15 +845,23 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas self.assertTrue(manual_enrollments[0].state_transition, UNENROLLED_TO_ENROLLED) def test_user_with_retired_email_in_csv(self): + """ + If the CSV contains email addresses which correspond with users which + have already been retired, confirm that the attempt returns invalid + email errors. + """ + + # This email address is re-used to create a retired account and another account. + conflicting_email = 'test_student@example.com' # prep a retired user - user = UserFactory.create(username='old_test_student', email='test_student@example.com') + user = UserFactory.create(username='old_test_student', email=conflicting_email) 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" \ + csv_content = "{email},{username},tester,USA".format(email=conflicting_email, username='new_test_student') uploaded_file = SimpleUploadedFile("temp.csv", csv_content) response = self.client.post(self.url, {'students_list': uploaded_file}) @@ -862,9 +870,9 @@ class TestInstructorAPIBulkAccountCreationAndEnrollment(SharedModuleStoreTestCas self.assertNotEquals(len(data['row_errors']), 0) self.assertEquals( data['row_errors'][0]['response'], - 'Invalid email {email}.'.format(email='test_student@example.com') + 'Invalid email {email}.'.format(email=conflicting_email) ) - self.assertFalse(User.objects.filter(email='test_student@example.com').exists()) + self.assertFalse(User.objects.filter(email=conflicting_email).exists()) def test_user_with_already_existing_username_in_csv(self): """ diff --git a/lms/djangoapps/instructor/views/tools.py b/lms/djangoapps/instructor/views/tools.py index 7484279d10..e3ce1ffe0b 100644 --- a/lms/djangoapps/instructor/views/tools.py +++ b/lms/djangoapps/instructor/views/tools.py @@ -9,8 +9,9 @@ from django.http import HttpResponseBadRequest from pytz import UTC from django.utils.translation import ugettext as _ from opaque_keys.edx.keys import UsageKey -from six import text_type +from six import text_type, string_types +from student.models import get_user_by_username_or_email from courseware.field_overrides import disable_overrides from courseware.models import StudentFieldOverride from courseware.student_field_overrides import clear_override_for_user, get_override_for_user, override_field_for_user @@ -50,7 +51,7 @@ def handle_dashboard_error(view): def strip_if_string(value): - if isinstance(value, basestring): + if isinstance(value, string_types): return value.strip() return value @@ -61,14 +62,12 @@ def get_student_from_identifier(unique_student_identifier): Returns the student object associated with `unique_student_identifier` - Raises User.DoesNotExist if no user object can be found. + Raises User.DoesNotExist if no user object can be found, the user was + retired, or the user is in the process of being retired. + + DEPRECATED: use student.models.get_user_by_username_or_email instead. """ - unique_student_identifier = strip_if_string(unique_student_identifier) - if "@" in unique_student_identifier: - student = User.objects.get(email=unique_student_identifier) - else: - student = User.objects.get(username=unique_student_identifier) - return student + return get_user_by_username_or_email(unique_student_identifier) def require_student_from_identifier(unique_student_identifier): diff --git a/lms/djangoapps/lms_migration/management/commands/create_user.py b/lms/djangoapps/lms_migration/management/commands/create_user.py index e3c6769479..acab51a35c 100644 --- a/lms/djangoapps/lms_migration/management/commands/create_user.py +++ b/lms/djangoapps/lms_migration/management/commands/create_user.py @@ -18,7 +18,7 @@ from django.core.management.base import BaseCommand from pytz import UTC from openedx.core.djangoapps.external_auth.models import ExternalAuthMap -from student.models import Registration, UserProfile +from student.models import Registration, UserProfile, email_exists_or_retired class MyCompleter(object): # Custom completer @@ -95,7 +95,7 @@ class Command(BaseCommand): while True: email = raw_input('email: ') - if User.objects.filter(email=email): + if email_exists_or_retired(email): print "email %s already taken" % email else: break diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 833d34916f..33f26d0b42 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/openedx/core/djangoapps/user_api/accounts/api.py @@ -13,7 +13,7 @@ from django.core.validators import validate_email, ValidationError from django.http import HttpResponseForbidden from six import text_type -from student.models import User, UserProfile, Registration +from student.models import User, UserProfile, Registration, email_exists_or_retired from student import forms as student_forms from student import views as student_views from util.model_utils import emit_setting_changed_event @@ -21,7 +21,6 @@ from util.password_policy_validators import validate_password from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.djangoapps.user_api import errors, accounts, forms, helpers -from openedx.core.djangoapps.user_api.accounts.utils import email_exists 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, @@ -693,7 +692,7 @@ def _validate_email_doesnt_exist(email): :return: None :raises: errors.AccountEmailAlreadyExists """ - if email is not None and email_exists(email): + if email is not None and email_exists_or_retired(email): raise errors.AccountEmailAlreadyExists(_(accounts.EMAIL_CONFLICT_MSG).format(email_address=email)) diff --git a/openedx/core/djangoapps/user_api/accounts/utils.py b/openedx/core/djangoapps/user_api/accounts/utils.py index 94d9e1d9d9..2967863861 100644 --- a/openedx/core/djangoapps/user_api/accounts/utils.py +++ b/openedx/core/djangoapps/user_api/accounts/utils.py @@ -15,7 +15,6 @@ 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 @@ -193,10 +192,3 @@ def generate_password(length=12, chars=string.letters + string.digits): password += choice(string.letters) password += ''.join([choice(chars) for _i in xrange(length - 2)]) return password - - -def email_exists(email): - """ - Check an email against the User model for existence. - """ - return User.objects.filter(email=email).exists() or is_email_retired(email) diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index 357f9ad861..13529a6518 100644 --- a/openedx/core/djangoapps/user_api/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/tests/test_views.py @@ -20,12 +20,15 @@ from six import text_type from social_django.models import UserSocialAuth, Partial from django_comment_common import models +from openedx.core.djangoapps.user_api.accounts.tests.test_views import RetirementTestCase +from openedx.core.djangoapps.user_api.models import UserRetirementStatus from openedx.core.djangoapps.site_configuration.helpers import get_value from openedx.core.lib.api.test_utils import ApiTestCase, TEST_API_KEY from openedx.core.lib.time_zone_utils import get_display_time_zone from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms from student.tests.factories import UserFactory +from student.models import get_retired_email_by_email from third_party_auth.tests.testutil import simulate_running_pipeline, ThirdPartyAuthTestMixin from third_party_auth.tests.utils import ( ThirdPartyOAuthTestMixin, ThirdPartyOAuthTestMixinFacebook, ThirdPartyOAuthTestMixinGoogle @@ -777,7 +780,7 @@ class PasswordResetViewTest(UserAPITestCase): @ddt.ddt @skip_unless_lms -class RegistrationViewValidationErrorTest(ThirdPartyAuthTestMixin, UserAPITestCase): +class RegistrationViewValidationErrorTest(ThirdPartyAuthTestMixin, UserAPITestCase, RetirementTestCase): """ Tests for catching duplicate email and username validation errors within the registration end-points of the User API. @@ -800,6 +803,99 @@ class RegistrationViewValidationErrorTest(ThirdPartyAuthTestMixin, UserAPITestCa super(RegistrationViewValidationErrorTest, self).setUp() self.url = reverse("user_api_registration") + def _retireRequestUser(self): + """ + Very basic user retirement initiation, logic copied form DeactivateLogoutView. This only lands the user in + PENDING, simulating a retirement request only. + """ + user = User.objects.get(username=self.USERNAME) + UserRetirementStatus.create_retirement(user) + user.email = get_retired_email_by_email(user.email) + user.set_unusable_password() + user.save() + + @mock.patch('openedx.core.djangoapps.user_api.views.check_account_exists') + def test_register_retired_email_validation_error(self, dummy_check_account_exists): + dummy_check_account_exists.return_value = [] + # Register the first user + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": self.NAME, + "username": self.USERNAME, + "password": self.PASSWORD, + "honor_code": "true", + }) + self.assertHttpOK(response) + + # Initiate retirement for the above user: + self._retireRequestUser() + + # Try to create a second user with the same email address as the retired user + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": "Someone Else", + "username": "someone_else", + "password": self.PASSWORD, + "honor_code": "true", + }) + self.assertEqual(response.status_code, 400) + response_json = json.loads(response.content) + self.assertEqual( + response_json, + { + "email": [{ + "user_message": ( + "It looks like {} belongs to an existing account. " + "Try again with a different email address." + ).format( + self.EMAIL + ) + }] + } + ) + + def test_register_retired_email_validation_error_no_bypass_check_account_exists(self): + """ + This test is the same as above, except it doesn't bypass check_account_exists. Not bypassing this function + results in the same error message, but a 409 status code rather than 400. + """ + # Register the first user + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": self.NAME, + "username": self.USERNAME, + "password": self.PASSWORD, + "honor_code": "true", + }) + self.assertHttpOK(response) + + # Initiate retirement for the above user: + self._retireRequestUser() + + # Try to create a second user with the same email address as the retired user + response = self.client.post(self.url, { + "email": self.EMAIL, + "name": "Someone Else", + "username": "someone_else", + "password": self.PASSWORD, + "honor_code": "true", + }) + self.assertEqual(response.status_code, 409) + response_json = json.loads(response.content) + self.assertEqual( + response_json, + { + "email": [{ + "user_message": ( + "It looks like {} belongs to an existing account. " + "Try again with a different email address." + ).format( + self.EMAIL + ) + }] + } + ) + @mock.patch('openedx.core.djangoapps.user_api.views.check_account_exists') def test_register_duplicate_email_validation_error(self, dummy_check_account_exists): dummy_check_account_exists.return_value = []