fix: redact pending primary email before retirement deletion

common/djangoapps/student/models/user.py

@@ -904,14 +904,32 @@ 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
     """
     user = models.OneToOneField(User, unique=True, db_index=True, on_delete=models.CASCADE)
     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):
+        """
+        Redacts new_email then deletes records where ``field`` equals ``value``.
+
+        Returns True if deleted, False if no matching records found.
+        """
+        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@retired.invalid')
+        records_matching_user_value.delete()
+        return True
+
     def request_change(self, email):
         """Request a change to a user's email.
 

common/djangoapps/student/tests/test_email.py

@@ -10,6 +10,7 @@
 from django.contrib.auth.models import User  # 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,33 @@ 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
+
+        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@retired.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()

common/djangoapps/student/tests/test_models.py

@@ -11,6 +11,7 @@
 from django.contrib.auth.models import AnonymousUser, User  # 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,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_delete_by_user_value_redacts_pending_email_before_deletion(self):
+        """
+        Verify that delete_by_user_value redacts new_email before deletion.
+        """
+        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'] == 'redacted@retired.invalid'
+            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):  # pylint: disable=missing-class-docstring
 

common/djangoapps/student/views/management.py

@@ -963,14 +963,17 @@ def confirm_email_change(request, key):
 
         user.email = pec.new_email
         user.save()
-        pec.delete()
+
+        # Redacts pending email before deletion
+        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, user.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': user.email})
             transaction.set_rollback(True)
             return response
 

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
@@ -1510,6 +1511,34 @@ 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.
+        """
+        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['new_email'] == 'redacted@retired.invalid'
+
+            # 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

@@ -1046,18 +1046,13 @@ def cleanup(self, request):
             if len(usernames) != len(retirements):
                 raise UserRetirementStatus.DoesNotExist("Not all usernames exist in the COMPLETE state.")
 
-            # Redact PII fields first, then delete. In case an ETL tool is syncing data
-            # to a downstream data warehouse, and treats the deletes as soft-deletes,
-            # the data will have first been redacted, protecting the sensitive PII.
-            # Get the IDs of the retirements to update/delete
+            # Redact PII first, then delete. Protects against ETL soft-delete scenarios.
             retirement_ids = list(retirements.values_list('id', flat=True))
-            # Update by IDs
             UserRetirementStatus.objects.filter(id__in=retirement_ids).update(
                 original_username=redacted_username,
                 original_email=redacted_email,
                 original_name=redacted_name
             )
-            # Delete by IDs
             UserRetirementStatus.objects.filter(id__in=retirement_ids, current_state=complete_state).delete()
 
             return Response(status=status.HTTP_204_NO_CONTENT)
@@ -1170,7 +1165,7 @@ def post(self, request):
 
             self.retire_entitlement_support_detail(user)
 
-            # Retire misc. models that may contain PII of this user
+            # Retire misc. PII models. PendingEmailChange redacts before deletion for downstream safety.
             PendingEmailChange.delete_by_user_value(user, field="user")
             UserOrgTag.delete_by_user_value(user, field="user")