fix: redact pending primary email before retirement deletion

Author: ktyagiapphelix2u

common/djangoapps/student/models/user.py

@@ -913,19 +913,25 @@ class PendingEmailChange(DeletableByUserValue, models.Model):  # noqa: DJ008
     activation_key = models.CharField(('activation key'), max_length=32, unique=True, db_index=True)
 
     @classmethod
-    def redact_pending_email_by_user_value(cls, value, field):
+    def delete_by_user_value(cls, value, field):
         """
-        Redact pending email change fields for records matching ``field=value``.
-        This method is intended for retirement flows where downstream systems
-        may keep soft-deleted snapshots of these rows.
+        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
-        for record in records_matching_user_value:
-            record.new_email = get_retired_email_by_email(record.new_email)
-            record.save(update_fields=['new_email'])
+
+        # Redact new_email before deletion using bulk update
+        records_matching_user_value.update(new_email='[email protected]')
+        records_matching_user_value.delete()
         return True
 
     def request_change(self, email):

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'] == '[email protected]'
+        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()

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
@@ -30,7 +31,6 @@
     UserAttribute,
     UserCelebration,
     UserProfile,
-    get_retired_email_by_email,
 )
 from common.djangoapps.student.models_api import confirm_name_change, do_name_change_request, get_name
 from common.djangoapps.student.tests.factories import AccountRecoveryFactory, CourseEnrollmentFactory, UserFactory
@@ -601,24 +601,33 @@ 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_redact_by_user_redacts_pending_email_change_fields(self):
-        original_new_email = self.email_change.new_email
-        original_activation_key = self.email_change.activation_key
-        expected_retired_email = get_retired_email_by_email(original_new_email)
-        record_was_redacted = PendingEmailChange.redact_pending_email_by_user_value(self.user, field='user')
-        assert record_was_redacted
-        self.email_change.refresh_from_db()
-        assert self.email_change.new_email == expected_retired_email
-        assert self.email_change.activation_key == original_activation_key
-
-    def test_redact_by_user_no_effect_for_user_with_no_email_change(self):
-        original_new_email = self.email_change.new_email
-        original_activation_key = self.email_change.activation_key
-        record_was_redacted = PendingEmailChange.redact_pending_email_by_user_value(self.user2, field='user')
-        assert not record_was_redacted
-        self.email_change.refresh_from_db()
-        assert self.email_change.new_email == original_new_email
-        assert self.email_change.activation_key == original_activation_key
+    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 = '[email protected]'
+        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

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
 

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

@@ -10,8 +10,9 @@
 import ddt
 from django.contrib.auth.models import User  # lint-amnesty, pylint: disable=imported-auth-user
 from django.contrib.sites.models import Site
-from django.core import mail
 from django.core.cache import cache
+from django.core import mail
+from django.db.models.signals import pre_delete
 from django.test import TestCase
 from django.urls import reverse
 from opaque_keys.edx.keys import CourseKey
@@ -1365,10 +1366,8 @@ 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')
-    @mock.patch('openedx.core.djangoapps.user_api.accounts.views.PendingEmailChange.redact_pending_email_by_user_value')
     def test_retire_user(
         self,
-        mock_redact_pending_email,
         mock_remove_profile_images,
         mock_get_profile_image_names,
     ):
@@ -1402,7 +1401,6 @@ def test_retire_user(
 
         self._entitlement_support_detail_assertions()
 
-        mock_redact_pending_email.assert_called_once_with(self.test_user, field="user")
         assert not PendingEmailChange.objects.filter(user=self.test_user).exists()
         assert not UserOrgTag.objects.filter(user=self.test_user).exists()
 
@@ -1415,22 +1413,34 @@ def test_retire_user_twice_idempotent(self):
         fake_completed_retirement(self.test_user)
         self.post_and_assert_status(data)
 
-    @mock.patch('openedx.core.djangoapps.user_api.accounts.views.PendingEmailChange.delete_by_user_value')
-    def test_retire_user_redacts_pending_email_before_delete(self, mock_delete_pending_email):
-        pending_email_record = PendingEmailChange.objects.get(user=self.test_user)
-        pending_email_before_retirement = pending_email_record.new_email
-        expected_retired_pending_email = get_retired_email_by_email(pending_email_before_retirement)
+    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 = '[email protected]'
+        captured_state = {}
 
-        def _assert_redacted_then_delete(value, field):
-            pending_record = PendingEmailChange.objects.get(user=self.test_user)
-            assert pending_record.new_email == expected_retired_pending_email
-            pending_record.delete()
-            return True
+        def capture_before_delete(sender, instance, **kwargs):
+            """Capture email value before it's deleted."""
+            captured_state['new_email'] = instance.new_email
 
-        mock_delete_pending_email.side_effect = _assert_redacted_then_delete
-        data = {'username': self.original_username}
-        self.post_and_assert_status(data)
-        assert not PendingEmailChange.objects.filter(user=self.test_user).exists()
+        # 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):

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

@@ -1152,9 +1152,8 @@ def post(self, request):
             self.retire_entitlement_support_detail(user)
 
             # Retire misc. models that may contain PII of this user.
-            # Redact pending email before delete because downstream systems
-            # may preserve soft-deleted snapshots.
-            PendingEmailChange.redact_pending_email_by_user_value(user, field="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")