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

common/djangoapps/student/models/user.py

@@ -76,6 +76,9 @@
 USER_LOGGED_OUT_EVENT_NAME = 'edx.user.logout'
 USER_STREAK_UPDATED_EVENT_NAME = "edx.user.celebration.streak_updated"
 
+# Placeholder for soft-deleted pending secondary email records
+PENDING_SECONDARY_EMAIL_REDACTED_VALUE = 'redact-before-delete@redacted.com'
+
 
 class AnonymousUserId(models.Model):  # noqa: DJ008
     """
@@ -941,14 +944,37 @@ class PendingSecondaryEmailChange(DeletableByUserValue, models.Model):  # noqa:
     """
     This model keeps track of pending requested changes to a user's secondary email address.
 
-    .. pii: Contains new_secondary_email, not currently retired
+    .. pii: Contains new_secondary_email, redact and delete in `DeactivateLogoutView`
     .. pii_types: email_address
-    .. pii_retirement: retained
+    .. pii_retirement: local_api
     """
     user = models.OneToOneField(User, unique=True, db_index=True, on_delete=models.CASCADE)
     new_secondary_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 redact_and_delete_pending_secondary_email(cls, user_id):
+        """
+        Redact and delete a pending secondary email change row for a user.
+
+        Redacts the email before deletion so any downstream soft-delete mirror does
+        not retain the original secondary email address in the final row image.
+        """
+        pending_secondary_email_records = cls.objects.filter(user_id=user_id)
+        pending_secondary_email_ids = list(
+            pending_secondary_email_records.values_list('id', flat=True)
+        )
+        if not pending_secondary_email_ids:
+            return False
+
+        # Converting to query set by id ensures we redact and delete the appropriate records
+        pending_secondary_email_records_by_id = cls.objects.filter(id__in=pending_secondary_email_ids)
+        pending_secondary_email_records_by_id.update(
+            new_secondary_email=PENDING_SECONDARY_EMAIL_REDACTED_VALUE,
+        )
+        pending_secondary_email_records_by_id.delete()
+        return True
+
 
 class LoginFailures(models.Model):
     """
@@ -1695,7 +1721,7 @@ class AccountRecovery(models.Model):  # noqa: DJ008
     """
     Model for storing information for user's account recovery in case of access loss.
 
-    .. pii: the field named secondary_email contains pii, retired in the `DeactivateLogoutView`
+    .. pii: the field named secondary_email contains pii, redact and delete in the `DeactivateLogoutView`
     .. pii_types: email_address
     .. pii_retirement: local_api
     """
@@ -1728,19 +1754,30 @@ def update_recovery_email(self, email):
     @classmethod
     def retire_recovery_email(cls, user_id):
         """
-        Retire user's recovery/secondary email as part of GDPR Phase I.
-        Returns 'True'
+        Redact and delete user's recovery/secondary email.
+
+        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.
 
-        If an AccountRecovery record is found for this user it will be deleted,
-        if it is not found it is assumed this table has no PII for the given user.
+        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
         """
-        try:
-            cls.objects.get(user_id=user_id).delete()
-        except cls.DoesNotExist:
-            pass
+        account_recovery_records = cls.objects.filter(user_id=user_id)
+        account_recovery_ids = list(
+            account_recovery_records.values_list('id', flat=True)
+        )
+        if not account_recovery_ids:
+            return False
+
+        # Converting to query set by id ensures we redact and delete the appropriate records
+        account_recovery_records_by_id = cls.objects.filter(id__in=account_recovery_ids)
+        account_recovery_records_by_id.update(
+            secondary_email=f'redact-before-delete+{user_id}@redacted.com',
+        )
+        account_recovery_records_by_id.delete()
 
         return True
 

common/djangoapps/student/tests/test_models.py

@@ -23,12 +23,14 @@
 from common.djangoapps.student.models import (
     ALLOWEDTOENROLL_TO_ENROLLED,
     IS_MARKETABLE,
+    PENDING_SECONDARY_EMAIL_REDACTED_VALUE,
     AccountRecovery,
     CourseEnrollment,
     CourseEnrollmentAllowed,
     ManualEnrollmentAudit,
     PendingEmailChange,
     PendingNameChange,
+    PendingSecondaryEmailChange,
     UserAttribute,
     UserCelebration,
     UserProfile,
@@ -758,19 +760,66 @@ 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)
+        with CaptureQueriesContext(connection) as ctx:
+            AccountRecovery.retire_recovery_email(user_id=user.id)
+
+        assert_redact_before_delete(
+            [q['sql'] for q in ctx],
+            table=AccountRecovery._meta.db_table,
+            expected_redacted_value_list=[f'redact-before-delete+{user.id}@redacted.com'],
+        )
 
-        # Assert that there is no longer an AccountRecovery record for this user
         assert len(AccountRecovery.objects.filter(user_id=user.id)) == 0
 
+    def test_retire_recovery_email_when_no_record(self):
+        """
+        Assert retirement cleanup returns False when no account recovery row exists.
+        """
+        user = UserFactory()
+        assert AccountRecovery.retire_recovery_email(user_id=user.id) is False
+
+
+class TestPendingSecondaryEmailChange(TestCase):
+    """
+    Tests for retiring PendingSecondaryEmailChange records.
+    """
+
+    def test_redact_and_delete_pending_secondary_email(self):
+        """
+        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
+
+        with CaptureQueriesContext(connection) as ctx:
+            PendingSecondaryEmailChange.redact_and_delete_pending_secondary_email(user_id=user.id)
+
+        assert_redact_before_delete(
+            [query['sql'] for query in ctx],
+            table=PendingSecondaryEmailChange._meta.db_table,
+            expected_redacted_value_list=[PENDING_SECONDARY_EMAIL_REDACTED_VALUE],
+        )
+
+        assert len(PendingSecondaryEmailChange.objects.filter(user_id=user.id)) == 0
+
+    def test_redact_and_delete_pending_secondary_email_when_no_record(self):
+        """
+        Assert retirement cleanup returns False when no pending secondary row exists.
+        """
+        user = UserFactory()
+        assert PendingSecondaryEmailChange.redact_and_delete_pending_secondary_email(user_id=user.id) is False
+
 
 @ddt.ddt
 class TestUserPostSaveCallback(SharedModuleStoreTestCase):

common/djangoapps/student/views/management.py

@@ -70,6 +70,7 @@
     RecoveryEmailCreate,
 )
 from common.djangoapps.student.models import (  # pylint: disable=unused-import
+    PENDING_SECONDARY_EMAIL_REDACTED_VALUE,
     AccountRecovery,
     CourseEnrollment,
     EnrollmentNotAllowed,
@@ -890,6 +891,9 @@ def activate_secondary_email(request, key):
             'secondary_email': pending_secondary_email_change.new_secondary_email
         })
 
+    # Redact the pending email before deletion so downstream soft-delete mirrors do not retain the original address.
+    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()
 
     return render_to_response("secondary_email_change_successful.html")

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

@@ -31,6 +31,7 @@
     ManualEnrollmentAudit,
     PendingEmailChange,
     PendingNameChange,
+    PendingSecondaryEmailChange,
     Registration,
     SocialLink,
     UserProfile,
@@ -1395,6 +1396,18 @@ def setUp(self):
         UserOrgTagFactory.create(user=self.test_user, key='foo', value='bar')
         UserOrgTagFactory.create(user=self.test_user, key='cat', value='dog')
 
+        # Secondary email setup
+        PendingSecondaryEmailChange.objects.create(
+            user=self.test_user,
+            new_secondary_email='pending_secondary@example.com',
+            activation_key='test_activation_key_123'
+        )
+        AccountRecovery.objects.create(
+            user=self.test_user,
+            secondary_email='confirmed_secondary@example.com',
+            is_active=True
+        )
+
         CourseEnrollmentAllowedFactory.create(email=self.original_email)
 
         self.course_key = CourseKey.from_string('course-v1:edX+DemoX+Demo_Course')
@@ -1486,6 +1499,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)
@@ -1508,6 +1522,10 @@ def test_retire_user(self, mock_remove_profile_images, mock_get_profile_image_na
         assert not PendingEmailChange.objects.filter(user=self.test_user).exists()
         assert not UserOrgTag.objects.filter(user=self.test_user).exists()
 
+        # Verify secondary email models were cleaned
+        assert not PendingSecondaryEmailChange.objects.filter(user=self.test_user).exists()
+        assert not AccountRecovery.objects.filter(user=self.test_user).exists()
+
         assert not CourseEnrollmentAllowed.objects.filter(email=self.original_email).exists()
         assert not UnregisteredLearnerCohortAssignments.objects.filter(email=self.original_email).exists()
 

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

@@ -17,7 +17,13 @@
 from edx_django_utils.user import generate_password
 from social_django.models import UserSocialAuth
 
-from common.djangoapps.student.models import AccountRecovery, Registration, get_retired_email_by_email
+from common.djangoapps.student.models import (
+    AccountRecovery,
+    PendingEmailChange,
+    PendingSecondaryEmailChange,
+    Registration,
+    get_retired_email_by_email,
+)
 from openedx.core.djangoapps.site_configuration.models import SiteConfiguration
 from openedx.core.djangoapps.theming.helpers import get_config_value_from_site_or_settings, get_current_site
 from openedx.core.djangolib.oauth2_retirement_utils import retire_dot_oauth2_models
@@ -250,6 +256,10 @@ def create_retirement_request_and_deactivate_account(user):
     retire_dot_oauth2_models(user)
     AccountRecovery.retire_recovery_email(user.id)
 
+    # Delete any pending account changes for added safety around account lock-out.
+    PendingEmailChange.delete_by_user_value(user, field='user')
+    PendingSecondaryEmailChange.redact_and_delete_pending_secondary_email(user.id)
+
 
 def username_suffix_generator(suffix_length=4):
     """

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

@@ -36,12 +36,14 @@
 from wiki.models.pluginbase import RevisionPluginRevision
 
 from common.djangoapps.entitlements.models import CourseEntitlement
-from common.djangoapps.student.models import (  # pylint: disable=unused-import
+from common.djangoapps.student.models import (  # lint-amnesty, pylint: disable=unused-import
+    AccountRecovery,
     CourseEnrollmentAllowed,
     LoginFailures,
     ManualEnrollmentAudit,
     PendingEmailChange,
     PendingNameChange,
+    PendingSecondaryEmailChange,
     User,
     UserProfile,
     get_potentially_retired_user_by_username,
@@ -1173,6 +1175,8 @@ def post(self, request):
             # Retire misc. models that may contain PII of this 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)
+            AccountRecovery.retire_recovery_email(user.id)
 
             # Retire any objects linked to the user via their original email
             CourseEnrollmentAllowed.delete_by_user_value(original_email, field="email")
@@ -1190,6 +1194,7 @@ def post(self, request):
             user.last_name = ""
             user.is_active = False
             user.username = retired_username
+            user.email = retired_email
             user.save()
         except UserRetirementStatus.DoesNotExist:
             return Response(status=status.HTTP_404_NOT_FOUND)

openedx/core/djangoapps/user_api/management/commands/retire_user.py

@@ -6,11 +6,8 @@
 from django.core.management.base import BaseCommand, CommandError
 from django.db import transaction
 
-from common.djangoapps.student.models import AccountRecovery, Registration, get_retired_email_by_email
-from openedx.core.djangolib.oauth2_retirement_utils import retire_dot_oauth2_models
-
-from ...accounts.utils import redact_and_delete_social_auth
-from ...models import BulkUserRetirementConfig, UserRetirementStatus
+from ...accounts.utils import create_retirement_request_and_deactivate_account
+from ...models import BulkUserRetirementConfig
 
 logger = logging.getLogger(__name__)
 
@@ -142,22 +139,7 @@ def handle(self, *args, **options):
         try:
             with transaction.atomic():
                 for user in users:
-                    # Add user to retirement queue.
-                    UserRetirementStatus.create_retirement(user)
-                    # Redact and unlink LMS social auth accounts.
-                    redact_and_delete_social_auth(user.id)
-                    # Change LMS password & email
-                    user.email = get_retired_email_by_email(user.email)
-                    user.set_unusable_password()
-                    user.save()
-
-                    # TODO: Unlink social accounts & change password on each IDA.
-                    # Remove the activation keys sent by email to the user for account activation.
-                    Registration.objects.filter(user=user).delete()
-
-                    # Delete OAuth tokens associated with the user.
-                    retire_dot_oauth2_models(user)
-                    AccountRecovery.retire_recovery_email(user.id)
+                    create_retirement_request_and_deactivate_account(user)
         except KeyError:
             error_message = f'Username not specified {user}'
             logger.error(error_message)

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

@@ -6,6 +6,7 @@
 import csv
 import os
 from contextlib import contextmanager
+from unittest import mock
 
 import pytest
 from django.contrib.auth.models import User  # pylint: disable=imported-auth-user
@@ -25,7 +26,9 @@
 from openedx.core.djangoapps.user_api.accounts.tests.test_utils import (
     assert_update_before_delete,
 )
-from openedx.core.djangolib.testing.utils import skip_unless_lms  # pylint: disable=wrong-import-order
+from openedx.core.djangolib.testing.utils import (
+    skip_unless_lms,
+)
 
 from ...models import UserRetirementStatus
 
@@ -133,6 +136,21 @@ def test_retire_with_username_email_userfile(setup_retirement_states):  # pylint
 
 
 @skip_unless_lms
+@pytest.mark.usefixtures('setup_retirement_states')
+@mock.patch(
+    'openedx.core.djangoapps.user_api.management.commands.retire_user.create_retirement_request_and_deactivate_account'
+)
+def test_retire_user_calls_shared_deactivate_helper(mock_deactivate_helper):
+    """
+    Verify the command delegates retirement side effects to the shared helper.
+    """
+    user = UserFactory.create(username='user-cleanup', email='user-cleanup@example.com')
+
+    call_command('retire_user', username=user.username, user_email=user.email)
+
+    mock_deactivate_helper.assert_called_once_with(user)
+
+
 @pytest.mark.parametrize('social_auth_configs', [
     # Single SSO provider
     [