From 5b9bb005bdcfa90714902b82cbf2b6b4dd12328c Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Tue, 2 Oct 2018 14:09:22 -0400 Subject: [PATCH] Prevent retired email reuse via account settings PLAT-2334 --- .../core/djangoapps/user_api/accounts/api.py | 9 +----- .../accounts/tests/retirement_helpers.py | 32 +++++++++++++------ .../user_api/accounts/tests/test_api.py | 29 ++++++++++++++++- .../accounts/tests/test_retirement_views.py | 28 +++++++++------- .../user_api/accounts/tests/test_views.py | 1 - .../djangoapps/user_api/tests/test_views.py | 25 +++++---------- 6 files changed, 76 insertions(+), 48 deletions(-) diff --git a/openedx/core/djangoapps/user_api/accounts/api.py b/openedx/core/djangoapps/user_api/accounts/api.py index 6ffadb263f..50ec3920ff 100644 --- a/openedx/core/djangoapps/user_api/accounts/api.py +++ b/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): diff --git a/openedx/core/djangoapps/user_api/accounts/tests/retirement_helpers.py b/openedx/core/djangoapps/user_api/accounts/tests/retirement_helpers.py index b2bf9d1004..15d607df42 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/retirement_helpers.py +++ b/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 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 f36357e578..74fdd571c5 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_api.py +++ b/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): """ diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py index 678b1b7d63..1a3cc6c5fd 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py +++ b/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) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py index 6b484e9a4f..7672aea82a 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/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, diff --git a/openedx/core/djangoapps/user_api/tests/test_views.py b/openedx/core/djangoapps/user_api/tests/test_views.py index 97002dbfa4..37eda24941 100644 --- a/openedx/core/djangoapps/user_api/tests/test_views.py +++ b/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