Prevent retired email reuse via account settings

PLAT-2334

openedx/core/djangoapps/user_api/accounts/api.py

@@ -182,7 +182,7 @@ def update_account_settings(requesting_user, update, username=None):
         # Don't process with sending email to given new email, if it is already associated with
         # an account. User must see same success message with no error.
         # This is so that this endpoint cannot be used to determine if an email is valid or not.
-        changing_email = not _verify_email_does_not_already_exists(new_email)
+        changing_email = new_email and not email_exists_or_retired(new_email)
 
     # If the user asked to change full name, validate it
     if changing_full_name:
@@ -273,13 +273,6 @@ def update_account_settings(requesting_user, update, username=None):
             )
 
 
-def _verify_email_does_not_already_exists(new_email):
-    """
-    Return `True` if an account with given email already exists.
-    """
-    return User.objects.filter(email=new_email).count() != 0
-
-
 @helpers.intercept_errors(errors.UserAPIInternalError, ignore_errors=[errors.UserAPIRequestError])
 @transaction.atomic
 def create_account(username, password, email):

openedx/core/djangoapps/user_api/accounts/tests/retirement_helpers.py

@@ -65,7 +65,7 @@ def setup_retirement_states(scope="module"):  # pylint: disable=unused-argument
     RetirementState.objects.all().delete()
 
 
-def create_retirement_status(state=None, create_datetime=None):
+def create_retirement_status(user, state=None, create_datetime=None):
     """
     Helper method to create a RetirementStatus with useful defaults.
     Assumes that retirement states have been setup before calling.
@@ -73,7 +73,6 @@ def create_retirement_status(state=None, create_datetime=None):
     if create_datetime is None:
         create_datetime = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=8)
 
-    user = UserFactory()
     retirement = UserRetirementStatus.create_retirement(user)
     if state:
         retirement.current_state = state
@@ -84,9 +83,15 @@ def create_retirement_status(state=None, create_datetime=None):
     return retirement
 
 
-def _fake_logged_out_user(user):
-    # Simulate the initial logout retirement endpoint.
-    user.username = get_retired_username_by_username(user.username)
+def _fake_logged_out_user(user, retire_username=False):
+    """
+    Simulate the initial logout retirement endpoint.
+    """
+    # By default, do not change the username to the retired hash version because
+    # that is not what happens upon actual retirement requests immediately after
+    # logout.
+    if retire_username:
+        user.username = get_retired_username_by_username(user.username)
     user.email = get_retired_email_by_email(user.email)
     user.set_unusable_password()
     user.save()
@@ -98,8 +103,7 @@ def logged_out_retirement_request():
     Returns a UserRetirementStatus test fixture object that has been logged out and email-changed,
     which is the first step which happens to a user being added to the retirement queue.
     """
-    retirement = create_retirement_status()
-    _fake_logged_out_user(retirement.user)
+    retirement = fake_requested_retirement(UserFactory())
     return retirement
 
 
@@ -149,7 +153,7 @@ class RetirementTestCase(TestCase):
         return retirement_dict
 
     def _create_users_all_states(self):
-        return [create_retirement_status(state) for state in RetirementState.objects.all()]
+        return [create_retirement_status(UserFactory(), state=state) for state in RetirementState.objects.all()]
 
     def _get_non_dead_end_states(self):
         return [state for state in RetirementState.objects.filter(is_dead_end_state=False)]
@@ -158,6 +162,16 @@ class RetirementTestCase(TestCase):
         return [state for state in RetirementState.objects.filter(is_dead_end_state=True)]
 
 
+def fake_requested_retirement(user):
+    """
+    Attempt to make the given user appear to have requested retirement, and
+    nothing more.  Among other things, this will only land the user in PENDING.
+    """
+    _fake_logged_out_user(user, retire_username=False)
+    retirement = create_retirement_status(user)
+    return retirement
+
+
 def fake_completed_retirement(user):
     """
     Makes an attempt to put user for the given user into a "COMPLETED"
@@ -169,7 +183,7 @@ def fake_completed_retirement(user):
     """
     # Deactivate / logout and hash username & email
     UserSocialAuth.objects.filter(user_id=user.id).delete()
-    _fake_logged_out_user(user)
+    _fake_logged_out_user(user, retire_username=True)
     user.first_name = ''
     user.last_name = ''
     user.is_active = False

openedx/core/djangoapps/user_api/accounts/tests/test_api.py

@@ -32,6 +32,11 @@ from openedx.core.djangoapps.user_api.accounts.tests.testutils import (
     INVALID_USERNAMES,
     VALID_USERNAMES_UNICODE
 )
+from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import (  # pylint: disable=unused-import
+    RetirementTestCase,
+    fake_requested_retirement,
+    setup_retirement_states
+)
 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 (
     AccountEmailInvalid,
@@ -59,7 +64,7 @@ def mock_render_to_string(template_name, context):
 
 @attr(shard=2)
 @skip_unless_lms
-class TestAccountApi(UserSettingsEventTestMixin, TestCase):
+class TestAccountApi(UserSettingsEventTestMixin, RetirementTestCase):
     """
     These tests specifically cover the parts of the API methods that are not covered by test_views.py.
     This includes the specific types of error raised, and default behavior when optional arguments
@@ -228,6 +233,28 @@ class TestAccountApi(UserSettingsEventTestMixin, TestCase):
             update_account_settings(self.user, disabled_update)
         self.assertIn("Email address changes have been disabled", context_manager.exception.developer_message)
 
+    @patch.dict(settings.FEATURES, dict(ALLOW_EMAIL_ADDRESS_CHANGE=True))
+    def test_email_changes_blocked_on_retired_email(self):
+        """
+        Test that email address changes are rejected when an email associated with a *partially* retired account is
+        specified.
+        """
+        # First, record the original email addres of the primary user (the one seeking to update their email).
+        original_email = self.user.email
+
+        # Setup a partially retired user.  This user recently submitted a deletion request, but it has not been
+        # processed yet.
+        partially_retired_email = 'partially_retired@example.com'
+        partially_retired_user = UserFactory(email=partially_retired_email)
+        fake_requested_retirement(partially_retired_user)
+
+        # Attempt to change email to the one of the partially retired user.
+        rejected_update = {'email': partially_retired_email}
+        update_account_settings(self.user, rejected_update)
+
+        # No error should be thrown, and we need to check that the email update was skipped.
+        assert self.user.email == original_email
+
     @patch('openedx.core.djangoapps.user_api.accounts.serializers.AccountUserSerializer.save')
     def test_serializer_save_fails(self, serializer_save):
         """

openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py

@@ -260,7 +260,7 @@ class TestAccountRetireMailings(RetirementTestCase):
 
         # Should be created in parent setUpClass
         retiring_email_lists = RetirementState.objects.get(state_name='RETIRING_EMAIL_LISTS')
-        self.retirement = create_retirement_status(retiring_email_lists)
+        self.retirement = create_retirement_status(UserFactory(), state=retiring_email_lists)
         self.test_user = self.retirement.user
 
         self.url = reverse('accounts_retire_mailings')
@@ -460,7 +460,7 @@ class TestPartnerReportingPut(RetirementTestCase, ModuleStoreTestCase):
         Checks the simple success case of creating a user, enrolling in a course, and doing the partner
         report PUT. User should then have the appropriate row in UserRetirementPartnerReportingStatus
         """
-        retirement = create_retirement_status(self.partner_queue_state)
+        retirement = create_retirement_status(UserFactory(), state=self.partner_queue_state)
         for course in self.courses:
             CourseEnrollment.enroll(user=retirement.user, course_key=course.id)
 
@@ -471,7 +471,7 @@ class TestPartnerReportingPut(RetirementTestCase, ModuleStoreTestCase):
         """
         Runs the success test twice to make sure that re-running the step still succeeds.
         """
-        retirement = create_retirement_status(self.partner_queue_state)
+        retirement = create_retirement_status(UserFactory(), state=self.partner_queue_state)
         for course in self.courses:
             CourseEnrollment.enroll(user=retirement.user, course_key=course.id)
 
@@ -504,7 +504,7 @@ class TestPartnerReportingPut(RetirementTestCase, ModuleStoreTestCase):
         the enrollment.course.org. We now just use the enrollment.course_id.org
         since for this purpose we don't care if the course exists.
         """
-        retirement = create_retirement_status(self.partner_queue_state)
+        retirement = create_retirement_status(UserFactory(), state=self.partner_queue_state)
         user = retirement.user
         enrollment = CourseEnrollment.enroll(user=user, course_key=CourseKey.from_string('edX/Test201/2018_Fall'))
 
@@ -722,7 +722,7 @@ class TestAccountRetirementList(RetirementTestCase):
         Verify that users in dead end states are not returned
         """
         for state in self._get_dead_end_states():
-            create_retirement_status(state)
+            create_retirement_status(UserFactory(), state=state)
         self.assert_status_and_user_list([], states_to_request=self._get_non_dead_end_states())
 
     def test_users_retrieved_in_multiple_states(self):
@@ -731,7 +731,7 @@ class TestAccountRetirementList(RetirementTestCase):
         """
         multiple_states = ['PENDING', 'FORUMS_COMPLETE']
         for state in multiple_states:
-            create_retirement_status(RetirementState.objects.get(state_name=state))
+            create_retirement_status(UserFactory(), state=RetirementState.objects.get(state_name=state))
         data = {'cool_off_days': 0, 'states': multiple_states}
         response = self.client.get(self.url, data, **self.headers)
         self.assertEqual(response.status_code, status.HTTP_200_OK)
@@ -768,7 +768,11 @@ class TestAccountRetirementList(RetirementTestCase):
         pending_state = RetirementState.objects.get(state_name='PENDING')
         for days_back in range(1, days_back_to_test, -1):
             create_datetime = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=days_back)
-            retirements.append(create_retirement_status(state=pending_state, create_datetime=create_datetime))
+            retirements.append(create_retirement_status(
+                UserFactory(),
+                state=pending_state,
+                create_datetime=create_datetime
+            ))
 
         # Confirm we get the correct number and data back for each day we add to cool off days
         # For each day we add to `cool_off_days` we expect to get one fewer retirement.
@@ -891,7 +895,7 @@ class TestAccountRetirementsByStatusAndDate(RetirementTestCase):
         Verify that users in non-requested states are not returned
         """
         state = RetirementState.objects.get(state_name='PENDING')
-        create_retirement_status(state=state)
+        create_retirement_status(UserFactory(), state=state)
         self.assert_status_and_user_list([])
 
     def test_users_exist(self):
@@ -922,7 +926,7 @@ class TestAccountRetirementsByStatusAndDate(RetirementTestCase):
         # Create retirements for the last 10 days
         for days_back in range(0, 10):
             create_datetime = datetime.datetime.now(pytz.UTC) - datetime.timedelta(days=days_back)
-            ret = create_retirement_status(state=complete_state, create_datetime=create_datetime)
+            ret = create_retirement_status(UserFactory(), state=complete_state, create_datetime=create_datetime)
             retirements.append(self._retirement_to_dict(ret))
 
         # Go back in time adding days to the query, assert the correct retirements are present
@@ -1015,7 +1019,7 @@ class TestAccountRetirementRetrieve(RetirementTestCase):
         retirements = []
 
         for state in RetirementState.objects.all():
-            retirements.append(create_retirement_status(state))
+            retirements.append(create_retirement_status(UserFactory(), state=state))
 
         for retirement in retirements:
             values = self._retirement_to_dict(retirement)
@@ -1026,7 +1030,7 @@ class TestAccountRetirementRetrieve(RetirementTestCase):
         Simulate retrieving a retirement by the old username, after the name has been changed to the hashed one
         """
         pending_state = RetirementState.objects.get(state_name='PENDING')
-        retirement = create_retirement_status(pending_state)
+        retirement = create_retirement_status(UserFactory(), state=pending_state)
         original_username = retirement.user.username
 
         hashed_username = get_retired_username_by_username(original_username)
@@ -1049,7 +1053,7 @@ class TestAccountRetirementUpdate(RetirementTestCase):
         self.pending_state = RetirementState.objects.get(state_name='PENDING')
         self.locking_state = RetirementState.objects.get(state_name='LOCKING_ACCOUNT')
 
-        self.retirement = create_retirement_status(self.pending_state)
+        self.retirement = create_retirement_status(UserFactory(), state=self.pending_state)
         self.test_user = self.retirement.user
         self.test_superuser = SuperuserFactory()
         self.headers = build_jwt_headers(self.test_superuser)

openedx/core/djangoapps/user_api/accounts/tests/test_views.py

@@ -32,7 +32,6 @@ from student.models import (
     SocialLink,
     UserProfile,
     get_retired_username_by_username,
-    get_retired_email_by_email,
 )
 from student.tests.factories import (
     TEST_PASSWORD,

openedx/core/djangoapps/user_api/tests/test_views.py

@@ -20,7 +20,6 @@ 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_retirement_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
@@ -41,7 +40,11 @@ from ..accounts import (
     NAME_MAX_LENGTH, EMAIL_MIN_LENGTH, EMAIL_MAX_LENGTH,
     USERNAME_MIN_LENGTH, USERNAME_MAX_LENGTH, USERNAME_BAD_LENGTH_MSG
 )
-from ..accounts.tests.retirement_helpers import setup_retirement_states  # pylint: disable=unused-import
+from ..accounts.tests.retirement_helpers import (  # pylint: disable=unused-import
+    RetirementTestCase,
+    fake_requested_retirement,
+    setup_retirement_states
+)
 from ..accounts.api import get_account_settings
 from ..models import UserOrgTag
 from ..tests.factories import UserPreferenceFactory
@@ -804,18 +807,6 @@ 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.username = get_retired_username_by_username(user.username)
-        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 = []
@@ -830,7 +821,7 @@ class RegistrationViewValidationErrorTest(ThirdPartyAuthTestMixin, UserAPITestCa
         self.assertHttpOK(response)
 
         # Initiate retirement for the above user:
-        self._retireRequestUser()
+        fake_requested_retirement(User.objects.get(username=self.USERNAME))
 
         # Try to create a second user with the same email address as the retired user
         response = self.client.post(self.url, {
@@ -872,7 +863,7 @@ class RegistrationViewValidationErrorTest(ThirdPartyAuthTestMixin, UserAPITestCa
         self.assertHttpOK(response)
 
         # Initiate retirement for the above user:
-        self._retireRequestUser()
+        fake_requested_retirement(User.objects.get(username=self.USERNAME))
 
         # Try to create a second user with the same email address as the retired user
         response = self.client.post(self.url, {
@@ -910,7 +901,7 @@ class RegistrationViewValidationErrorTest(ThirdPartyAuthTestMixin, UserAPITestCa
         self.assertHttpOK(response)
 
         # Initiate retirement for the above user.
-        self._retireRequestUser()
+        fake_requested_retirement(User.objects.get(username=self.USERNAME))
 
         with mock.patch('openedx.core.djangoapps.user_authn.views.register.do_create_account') as dummy_do_create_acct:
             # do_create_account should *not* be called - the duplicate retired username