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

Author: ktyagiapphelix2u

common/djangoapps/student/models/user.py

@@ -954,7 +954,9 @@ def redact_and_delete_pending_secondary_email(cls, user_id):
             pending_secondary_email = cls.objects.get(user_id=user_id)
         except cls.DoesNotExist:
             return True
-        pending_secondary_email.new_secondary_email = f"redacted+{user_id}@redacted.com"
+        pending_secondary_email.new_secondary_email = get_retired_email_by_email(
+            pending_secondary_email.new_secondary_email
+        )
         pending_secondary_email.save(update_fields=['new_secondary_email'])
         pending_secondary_email.delete()
         return True
@@ -1743,8 +1745,8 @@ def retire_recovery_email(cls, user_id):
         If an AccountRecovery record is found for this user it will be redacted and
         deleted. If it is not found it is assumed this table has no PII for the given user.
 
-        Note: "Retire" here refers to retiring this user's data, not the recovery email
-        feature itself, which remains available for new accounts.
+        Note: In retire_recovery_email, "retire" means this is part of user retirement.
+        The recovery email itself remains available for use with future accounts.
 
         :param user_id: int
         :return: bool - True on success
@@ -1753,9 +1755,8 @@ def retire_recovery_email(cls, user_id):
             account_recovery = cls.objects.get(user_id=user_id)
         except cls.DoesNotExist:
             return True
-        account_recovery.secondary_email = f"redacted+{user_id}@redacted.com"
-        account_recovery.is_active = False
-        account_recovery.save(update_fields=['secondary_email', 'is_active'])
+        account_recovery.secondary_email = get_retired_email_by_email(account_recovery.secondary_email)
+        account_recovery.save(update_fields=['secondary_email'])
         account_recovery.delete()
 
         return True

common/djangoapps/student/tests/test_models.py

@@ -734,34 +734,61 @@ class TestAccountRecovery(TestCase):
 
     def test_retire_recovery_email(self):
         """
-        Assert that Account Record for a given user is deleted when `retire_recovery_email` is called
+        Assert that AccountRecovery secondary_email is redacted before the record is deleted.
         """
-        # Create user and associated recovery email record
         user = UserFactory()
-        AccountRecoveryFactory(user=user)
+        AccountRecoveryFactory(user=user, secondary_email='recovery@example.com')
         assert len(AccountRecovery.objects.filter(user_id=user.id)) == 1
 
-        # Retire recovery email
-        AccountRecovery.retire_recovery_email(user_id=user.id)
+        saved_emails = []
 
-        # Assert that there is no longer an AccountRecovery record for this user
+        original_save = AccountRecovery.save
+
+        def capture_save(instance, *args, **kwargs):
+            saved_emails.append(instance.secondary_email)
+            original_save(instance, *args, **kwargs)
+
+        with mock.patch.object(AccountRecovery, 'save', side_effect=capture_save):
+            AccountRecovery.retire_recovery_email(user_id=user.id)
+
+        # 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):
     """Tests for retiring PendingSecondaryEmailChange records."""
 
     def test_redact_and_delete_pending_secondary_email(self):
-        """Assert that pending secondary email records are deleted for retired users."""
+        """Assert that the pending secondary email is redacted before the record is deleted."""
         user = UserFactory()
         PendingSecondaryEmailChange.objects.create(
             user=user,
             new_secondary_email='new-secondary@example.com',
             activation_key='a' * 32,
         )
         assert len(PendingSecondaryEmailChange.objects.filter(user_id=user.id)) == 1
-        PendingSecondaryEmailChange.redact_and_delete_pending_secondary_email(user_id=user.id)
+
+        saved_emails = []
+
+        original_save = PendingSecondaryEmailChange.save
+
+        def capture_save(instance, *args, **kwargs):
+            saved_emails.append(instance.new_secondary_email)
+            original_save(instance, *args, **kwargs)
+
+        with mock.patch.object(PendingSecondaryEmailChange, 'save', side_effect=capture_save):
+            PendingSecondaryEmailChange.redact_and_delete_pending_secondary_email(user_id=user.id)
+
+        # 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

@@ -84,7 +84,6 @@
     UserStanding,
     create_comments_service_user,  # noqa: F401
     email_exists_or_retired,  # noqa: F401
-    get_retired_email_by_email,
 )
 from common.djangoapps.student.signals import REFUND_ORDER, USER_EMAIL_CHANGED
 from common.djangoapps.student.toggles import should_redirect_to_courseware_after_enrollment
@@ -891,9 +890,12 @@ def activate_secondary_email(request, key):
             'secondary_email': pending_secondary_email_change.new_secondary_email
         })
 
-    pending_secondary_email_change.new_secondary_email = get_retired_email_by_email(
-        pending_secondary_email_change.new_secondary_email
-    )
+    # Overwrite the pending email with a neutral placeholder before deletion so
+    # 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; a hashed retirement-style value would
+    # produce false-positive retirement signals in downstream systems.
+    pending_secondary_email_change.new_secondary_email = "completed@delete-pending.com"
     pending_secondary_email_change.save(update_fields=['new_secondary_email'])
     pending_secondary_email_change.delete()
 

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

@@ -1154,11 +1154,7 @@ def post(self, request):
             self.retire_entitlement_support_detail(user)
 
             # Retire misc. models that may contain PII of this user
-            # Redact pending email change before deletion to prevent plaintext sync to Snowflake
-            pending_email = PendingEmailChange.objects.filter(user=user).first()
-            if pending_email:
-                pending_email.new_email = get_retired_email_by_email(pending_email.new_email)
-                pending_email.save(update_fields=['new_email'])
+            PendingEmailChange.redact_pending_email_by_user_value(user, field="user")
             PendingEmailChange.delete_by_user_value(user, field="user")
             UserOrgTag.delete_by_user_value(user, field="user")
             PendingSecondaryEmailChange.redact_and_delete_pending_secondary_email(user.id)

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

@@ -5,6 +5,7 @@
 
 import csv
 import os
+from unittest import mock
 
 import pytest
 from django.contrib.auth.models import User  # lint-amnesty, pylint: disable=imported-auth-user
@@ -120,6 +121,19 @@ def test_retire_user_cleans_pending_secondary_email(setup_retirement_states):  #
     )
     assert PendingSecondaryEmailChange.objects.filter(user=user).exists()
 
-    call_command('retire_user', username=user.username, user_email=user.email)
+    saved_emails = []
+    original_save = PendingSecondaryEmailChange.save
 
+    def capture_save(instance, *args, **kwargs):
+        saved_emails.append(instance.new_secondary_email)
+        original_save(instance, *args, **kwargs)
+
+    with mock.patch.object(PendingSecondaryEmailChange, 'save', side_effect=capture_save):
+        call_command('retire_user', username=user.username, user_email=user.email)
+
+    # 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]