fix: retirement PII leaks by redacting pending secondary email/name data

Author: ktyagiapphelix2u

common/djangoapps/student/tests/test_models.py

@@ -10,8 +10,10 @@
 from django.conf import settings
 from django.contrib.auth.models import AnonymousUser, User  # lint-amnesty, pylint: disable=imported-auth-user
 from django.core.cache import cache
+from django.db import connection
 from django.db.models.functions import Lower
 from django.test import TestCase, override_settings
+from django.test.utils import CaptureQueriesContext
 from edx_toggles.toggles.testutils import override_waffle_flag
 from freezegun import freeze_time
 from opaque_keys.edx.keys import CourseKey
@@ -740,24 +742,21 @@ def test_retire_recovery_email(self):
         AccountRecoveryFactory(user=user, secondary_email='recovery@example.com')
         assert len(AccountRecovery.objects.filter(user_id=user.id)) == 1
 
-        from django.db.models.signals import pre_delete
-        saved_emails = []
-
-        def capture_pre_delete(sender, instance, **kwargs):
-            saved_emails.append(instance.secondary_email)
-
-        pre_delete.connect(capture_pre_delete, sender=AccountRecovery)
-        try:
+        with CaptureQueriesContext(connection) as ctx:
             AccountRecovery.retire_recovery_email(user_id=user.id)
-        finally:
-            pre_delete.disconnect(capture_pre_delete, sender=AccountRecovery)
+
+        sql_list = [query['sql'].upper() for query in ctx]
+        table_name = 'AUTH_ACCOUNTRECOVERY'
+        update_indices = [i for i, sql in enumerate(sql_list) if 'UPDATE' in sql and table_name in sql]
+        delete_indices = [i for i, sql in enumerate(sql_list) if 'DELETE' in sql and table_name in sql]
+        assert update_indices, f'Expected at least one UPDATE on {table_name}'
+        assert delete_indices, f'Expected at least one DELETE on {table_name}'
+        assert any(update_idx < delete_idx for update_idx in update_indices for delete_idx in delete_indices), (
+            'Expected at least one UPDATE to precede at least one DELETE'
+        )
 
         # Record must be gone
         assert len(AccountRecovery.objects.filter(user_id=user.id)) == 0
-        # Redacted value was saved (not the original) before deletion
-        assert len(saved_emails) == 1
-        assert saved_emails[0] != 'recovery@example.com'
-        assert 'example.com' not in saved_emails[0]
 
 
 class TestPendingSecondaryEmailChange(TestCase):
@@ -773,24 +772,21 @@ def test_redact_and_delete_pending_secondary_email(self):
         )
         assert len(PendingSecondaryEmailChange.objects.filter(user_id=user.id)) == 1
 
-        from django.db.models.signals import pre_delete
-        saved_emails = []
-
-        def capture_pre_delete(sender, instance, **kwargs):
-            saved_emails.append(instance.new_secondary_email)
-
-        pre_delete.connect(capture_pre_delete, sender=PendingSecondaryEmailChange)
-        try:
+        with CaptureQueriesContext(connection) as ctx:
             PendingSecondaryEmailChange.redact_and_delete_pending_secondary_email(user_id=user.id)
-        finally:
-            pre_delete.disconnect(capture_pre_delete, sender=PendingSecondaryEmailChange)
+
+        sql_list = [query['sql'].upper() for query in ctx]
+        table_name = 'STUDENT_PENDINGSECONDARYEMAILCHANGE'
+        update_indices = [i for i, sql in enumerate(sql_list) if 'UPDATE' in sql and table_name in sql]
+        delete_indices = [i for i, sql in enumerate(sql_list) if 'DELETE' in sql and table_name in sql]
+        assert update_indices, f'Expected at least one UPDATE on {table_name}'
+        assert delete_indices, f'Expected at least one DELETE on {table_name}'
+        assert any(update_idx < delete_idx for update_idx in update_indices for delete_idx in delete_indices), (
+            'Expected at least one UPDATE to precede at least one DELETE'
+        )
 
         # Record must be gone
         assert len(PendingSecondaryEmailChange.objects.filter(user_id=user.id)) == 0
-        # Redacted value was saved (not the original) before deletion
-        assert len(saved_emails) == 1
-        assert saved_emails[0] != 'new-secondary@example.com'
-        assert 'example.com' not in saved_emails[0]
 
     def test_redact_and_delete_pending_secondary_email_when_no_record(self):
         """Assert retirement cleanup returns True when no pending secondary row exists."""

common/djangoapps/student/views/management.py

@@ -118,6 +118,8 @@
 
 log = logging.getLogger("edx.student")
 
+PENDING_SECONDARY_EMAIL_REDACTED_VALUE = 'redacted@retired.invalid'
+
 AUDIT_LOG = logging.getLogger("audit")
 ReverifyInfo = namedtuple(
     'ReverifyInfo',
@@ -894,7 +896,7 @@ def activate_secondary_email(request, key):
     # that any downstream soft-delete mirror does not retain the real address.
     # We do NOT use get_retired_email_by_email() here because this is a normal
     # user action, not a retirement flow.
-    pending_secondary_email_change.new_secondary_email = "redacted@retired.invalid"
+    pending_secondary_email_change.new_secondary_email = PENDING_SECONDARY_EMAIL_REDACTED_VALUE
     pending_secondary_email_change.save(update_fields=['new_secondary_email'])
     pending_secondary_email_change.delete()
 

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

@@ -1408,6 +1408,7 @@ def test_retire_user(self, mock_remove_profile_images, mock_get_profile_image_na
             'last_name': '',
             'is_active': False,
             'username': self.retired_username,
+            'email': self.retired_email,
         }
         for field, expected_value in expected_user_values.items():
             assert expected_value == getattr(self.test_user, field)

openedx/core/djangoapps/user_api/management/tests/test_retire_user.py

@@ -9,7 +9,8 @@
 import pytest
 from django.contrib.auth.models import User  # lint-amnesty, pylint: disable=imported-auth-user
 from django.core.management import CommandError, call_command
-from django.db.models.signals import pre_delete
+from django.db import connection
+from django.test.utils import CaptureQueriesContext
 
 from common.djangoapps.student.models import PendingSecondaryEmailChange
 from common.djangoapps.student.tests.factories import UserFactory  # lint-amnesty, pylint: disable=wrong-import-order
@@ -121,20 +122,18 @@ def test_retire_user_cleans_pending_secondary_email(setup_retirement_states):  #
     )
     assert PendingSecondaryEmailChange.objects.filter(user=user).exists()
 
-    saved_emails = []
-
-    def capture_pre_delete(sender, instance, **kwargs):
-        saved_emails.append(instance.new_secondary_email)
-
-    pre_delete.connect(capture_pre_delete, sender=PendingSecondaryEmailChange)
-    try:
+    with CaptureQueriesContext(connection) as ctx:
         call_command('retire_user', username=user.username, user_email=user.email)
-    finally:
-        pre_delete.disconnect(capture_pre_delete, sender=PendingSecondaryEmailChange)
+
+    sql_list = [query['sql'].upper() for query in ctx]
+    table_name = 'STUDENT_PENDINGSECONDARYEMAILCHANGE'
+    update_indices = [i for i, sql in enumerate(sql_list) if 'UPDATE' in sql and table_name in sql]
+    delete_indices = [i for i, sql in enumerate(sql_list) if 'DELETE' in sql and table_name in sql]
+    assert update_indices, f'Expected at least one UPDATE on {table_name}'
+    assert delete_indices, f'Expected at least one DELETE on {table_name}'
+    assert any(update_idx < delete_idx for update_idx in update_indices for delete_idx in delete_indices), (
+        'Expected at least one UPDATE to precede at least one DELETE'
+    )
 
     # Record must be deleted
     assert not PendingSecondaryEmailChange.objects.filter(user=user).exists()
-    # Redacted value was saved before deletion — not the original plaintext email
-    assert len(saved_emails) == 1
-    assert saved_emails[0] != 'pending-secondary@example.com'
-    assert 'example.com' not in saved_emails[0]