diff --git a/common/djangoapps/student/models/user.py b/common/djangoapps/student/models/user.py index 6b7cdb7e761a..1c6f826aa5c0 100644 --- a/common/djangoapps/student/models/user.py +++ b/common/djangoapps/student/models/user.py @@ -904,7 +904,7 @@ class PendingEmailChange(DeletableByUserValue, models.Model): # noqa: DJ008 """ This model keeps track of pending requested changes to a user's email address. - .. pii: Contains new_email, retired in AccountRetirementView + .. pii: Contains new_email, redacted then deleted in AccountRetirementView .. pii_types: email_address .. pii_retirement: local_api """ @@ -912,6 +912,28 @@ class PendingEmailChange(DeletableByUserValue, models.Model): # noqa: DJ008 new_email = models.CharField(blank=True, max_length=255, db_index=True) activation_key = models.CharField(('activation key'), max_length=32, unique=True, db_index=True) + @classmethod + def delete_by_user_value(cls, value, field): + """ + Deletes instances of this model where ``field`` equals ``value``. + + Automatically redacts new_email before deletion to ensure PII is cleared. + Uses bulk ORM update for efficiency. + + Returns True if any instances were deleted. + Returns False otherwise. + """ + filter_kwargs = {field: value} + records_matching_user_value = cls.objects.filter(**filter_kwargs) + + if not records_matching_user_value.exists(): + return False + + # Redact new_email before deletion using bulk update + records_matching_user_value.update(new_email='redacted@redacted.invalid') + records_matching_user_value.delete() + return True + def request_change(self, email): """Request a change to a user's email. diff --git a/common/djangoapps/student/tests/test_email.py b/common/djangoapps/student/tests/test_email.py index d9aa2dd19662..de82ae4be2ab 100644 --- a/common/djangoapps/student/tests/test_email.py +++ b/common/djangoapps/student/tests/test_email.py @@ -10,6 +10,7 @@ from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core import mail from django.db import transaction +from django.db.models.signals import pre_delete from django.http import HttpResponse from django.test import TransactionTestCase, override_settings from django.test.client import RequestFactory @@ -608,6 +609,34 @@ def test_successful_email_change(self, test_body_type, test_marketing_enabled, m assert self.pending_change_request.new_email == User.objects.get(username=self.user.username).email assert PendingEmailChange.objects.count() == 0 + @skip_unless_lms + @patch('common.djangoapps.student.signals.receivers.EmailChangeMiddleware.register_email_change') + @patch('common.djangoapps.student.views.management.ace') + def test_successful_email_change_redacts_pending_email_before_delete(self, ace_mail, mock_register): # pylint: disable=unused-argument + original_email = self.user.email + expected_new_email = self.pending_change_request.new_email + captured_state = {} + + def capture_before_delete(sender, instance, **kwargs): + captured_state['new_email'] = instance.new_email + + ace_mail.send.side_effect = [None, None] + pre_delete.connect(capture_before_delete, sender=PendingEmailChange) + try: + response = confirm_email_change(self.request, self.key) + finally: + pre_delete.disconnect(capture_before_delete, sender=PendingEmailChange) + + assert response.status_code == 200 + assert mock_render_to_response('email_change_successful.html', { + 'old_email': original_email, + 'new_email': expected_new_email, + }).content.decode('utf-8') == response.content.decode('utf-8') + assert captured_state['new_email'] == 'redacted@redacted.invalid' + assert User.objects.get(username=self.user.username).email == expected_new_email + assert PendingEmailChange.objects.count() == 0 + assert ace_mail.send.call_count == 2 + @patch('common.djangoapps.student.views.PendingEmailChange.objects.get', Mock(side_effect=TestException)) def test_always_rollback(self): connection = transaction.get_connection() diff --git a/common/djangoapps/student/tests/test_models.py b/common/djangoapps/student/tests/test_models.py index 09a1d35fa424..e49bfe9387f9 100644 --- a/common/djangoapps/student/tests/test_models.py +++ b/common/djangoapps/student/tests/test_models.py @@ -11,6 +11,7 @@ from django.contrib.auth.models import AnonymousUser, User # lint-amnesty, pylint: disable=imported-auth-user from django.core.cache import cache from django.db.models.functions import Lower +from django.db.models.signals import pre_delete from django.test import TestCase, override_settings from edx_toggles.toggles.testutils import override_waffle_flag from freezegun import freeze_time @@ -600,6 +601,34 @@ def test_delete_by_user_no_effect_for_user_with_no_email_change(self): assert not record_was_deleted assert 1 == len(PendingEmailChange.objects.all()) + def test_delete_by_user_value_redacts_pending_email_before_deletion(self): + """ + Verify that delete_by_user_value redacts new_email before deletion. + """ + expected_redacted_email = 'redacted@redacted.invalid' + captured_state = {} + + def capture_before_delete(sender, instance, **kwargs): + """ + Capture email and activation_key before deletion. + """ + captured_state['new_email'] = instance.new_email + captured_state['activation_key'] = instance.activation_key + + pre_delete.connect(capture_before_delete, sender=PendingEmailChange) + try: + assert PendingEmailChange.objects.filter(user=self.user).exists() + + record_was_deleted = PendingEmailChange.delete_by_user_value(self.user, field='user') + assert record_was_deleted + + assert captured_state['new_email'] == expected_redacted_email + assert captured_state['activation_key'] == self.email_change.activation_key + + assert not PendingEmailChange.objects.filter(user=self.user).exists() + finally: + pre_delete.disconnect(capture_before_delete, sender=PendingEmailChange) + class TestCourseEnrollmentAllowed(ModuleStoreTestCase): # lint-amnesty, pylint: disable=missing-class-docstring diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index cf7948b448bb..e23b13c320dd 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -961,16 +961,20 @@ def confirm_email_change(request, key): transaction.set_rollback(True) return response - user.email = pec.new_email + new_email = pec.new_email + user.email = new_email user.save() - pec.delete() + + # Use model-level deletion to redact pending email before delete. + PendingEmailChange.delete_by_user_value(user, field="user") + # And send it to the new email... - msg.recipient = Recipient(user.id, pec.new_email) + msg.recipient = Recipient(user.id, new_email) try: ace.send(msg) except Exception: # pylint: disable=broad-except log.warning('Unable to send confirmation email to new address', exc_info=True) - response = render_to_response("email_change_failed.html", {'email': pec.new_email}) + response = render_to_response("email_change_failed.html", {'email': new_email}) transaction.set_rollback(True) return response 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 4f4309934374..b461c0c2c9a5 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 @@ -13,6 +13,7 @@ from django.core import mail from django.core.cache import cache from django.db import connection +from django.db.models.signals import pre_delete from django.test import TestCase from django.test.utils import CaptureQueriesContext from django.urls import reverse @@ -1467,7 +1468,11 @@ def test_retire_user_where_username_not_provided(self): @mock.patch('openedx.core.djangoapps.user_api.accounts.views.get_profile_image_names') @mock.patch('openedx.core.djangoapps.user_api.accounts.views.remove_profile_images') - def test_retire_user(self, mock_remove_profile_images, mock_get_profile_image_names): + def test_retire_user( + self, + mock_remove_profile_images, + mock_get_profile_image_names, + ): data = {'username': self.original_username} self.post_and_assert_status(data) @@ -1510,6 +1515,35 @@ def test_retire_user_twice_idempotent(self): fake_completed_retirement(self.test_user) self.post_and_assert_status(data) + def test_retire_user_redacts_pending_email_before_delete(self): + """ + Verify that delete_by_user_value redacts new_email using bulk update before deletion. + """ + expected_redacted_email = 'redacted@redacted.invalid' + captured_state = {} + + def capture_before_delete(sender, instance, **kwargs): + """Capture email value before it's deleted.""" + captured_state['new_email'] = instance.new_email + + # Connect signal to capture pre-delete state + pre_delete.connect(capture_before_delete, sender=PendingEmailChange) + try: + # Verify the record exists with original email before retirement + assert PendingEmailChange.objects.filter(user=self.test_user).exists() + + # Retire the user + data = {'username': self.original_username} + self.post_and_assert_status(data) + + # Verify the redaction happened before deletion + assert captured_state.get('new_email') == expected_redacted_email + + # Verify the record was deleted + assert not PendingEmailChange.objects.filter(user=self.test_user).exists() + finally: + pre_delete.disconnect(capture_before_delete, sender=PendingEmailChange) + @mock.patch('openedx.core.djangoapps.user_api.accounts.views.USER_RETIRE_LMS_CRITICAL') def test_retirement_sends_critical_signal_with_retirement_data(self, mock_signal): """ diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index c900f0457389..842b612fab8a 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1170,7 +1170,9 @@ def post(self, request): self.retire_entitlement_support_detail(user) - # Retire misc. models that may contain PII of this user + # Retire misc. models that may contain PII of this user. + # PendingEmailChange.delete_by_user_value() automatically redacts new_email + # before deletion because downstream systems may preserve soft-deleted snapshots. PendingEmailChange.delete_by_user_value(user, field="user") UserOrgTag.delete_by_user_value(user, field="user")