From 1e6614b10029cf15460cc8beacabf1b225466aef Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Thu, 23 Apr 2026 10:55:56 +0000 Subject: [PATCH 1/3] fix: retirement PII leaks by redacting pending secondary email/name data --- common/djangoapps/student/models/user.py | 37 +++++++++++++++---- .../djangoapps/student/tests/test_models.py | 22 +++++++++++ common/djangoapps/student/views/management.py | 5 +++ .../accounts/tests/test_retirement_views.py | 35 ++++++++++++++++++ .../djangoapps/user_api/accounts/utils.py | 8 +++- .../djangoapps/user_api/accounts/views.py | 10 +++++ .../management/commands/retire_user.py | 8 +++- .../management/tests/test_retire_user.py | 16 ++++++++ 8 files changed, 131 insertions(+), 10 deletions(-) diff --git a/common/djangoapps/student/models/user.py b/common/djangoapps/student/models/user.py index 6b7cdb7e761a..da3032a66920 100644 --- a/common/djangoapps/student/models/user.py +++ b/common/djangoapps/student/models/user.py @@ -934,14 +934,31 @@ 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, redacted 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_pending_secondary_email(cls, user_id): + """ + Redact 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. + """ + try: + 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.save(update_fields=['new_secondary_email']) + pending_secondary_email.delete() + return True + class LoginFailures(models.Model): """ @@ -1688,7 +1705,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, redacted in the `DeactivateLogoutView` .. pii_types: email_address .. pii_retirement: local_api """ @@ -1721,19 +1738,23 @@ 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. + Redact user's recovery/secondary email as part of GDPR Phase I. Returns 'True' - 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. + 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. :param user_id: int :return: bool """ try: - cls.objects.get(user_id=user_id).delete() + account_recovery = cls.objects.get(user_id=user_id) except cls.DoesNotExist: - pass + 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.delete() return True diff --git a/common/djangoapps/student/tests/test_models.py b/common/djangoapps/student/tests/test_models.py index 09a1d35fa424..c5ce4bd55a1e 100644 --- a/common/djangoapps/student/tests/test_models.py +++ b/common/djangoapps/student/tests/test_models.py @@ -27,6 +27,7 @@ ManualEnrollmentAudit, PendingEmailChange, PendingNameChange, + PendingSecondaryEmailChange, UserAttribute, UserCelebration, UserProfile, @@ -747,6 +748,27 @@ def test_retire_recovery_email(self): assert len(AccountRecovery.objects.filter(user_id=user.id)) == 0 +class TestPendingSecondaryEmailChange(TestCase): + """Tests for retiring PendingSecondaryEmailChange records.""" + + def test_redact_pending_secondary_email(self): + """Assert that pending secondary email records are deleted for retired users.""" + 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_pending_secondary_email(user_id=user.id) + assert len(PendingSecondaryEmailChange.objects.filter(user_id=user.id)) == 0 + + def test_redact_pending_secondary_email_when_no_record(self): + """Assert retirement cleanup returns True when no pending secondary row exists.""" + user = UserFactory() + assert PendingSecondaryEmailChange.redact_pending_secondary_email(user_id=user.id) is True + + @ddt.ddt class TestUserPostSaveCallback(SharedModuleStoreTestCase): """ diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index cf7948b448bb..2e5bb5a4d61c 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -84,6 +84,7 @@ 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 @@ -890,6 +891,10 @@ 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 + ) + pending_secondary_email_change.save(update_fields=['new_secondary_email']) pending_secondary_email_change.delete() return render_to_response("secondary_email_change_successful.html") diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py index dfc07b643b9d..13a3f1e814c5 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py @@ -29,6 +29,7 @@ ManualEnrollmentAudit, PendingEmailChange, PendingNameChange, + PendingSecondaryEmailChange, Registration, SocialLink, UserProfile, @@ -233,6 +234,24 @@ def test_user_can_deactivate_secondary_email(self): # Assert that there is no longer a secondary/recovery email for test user assert len(AccountRecovery.objects.filter(user_id=self.test_user.id)) == 0 + def test_user_can_deactivate_pending_secondary_email_change(self): + """ + Verify that pending secondary email change records are removed when a user retires. + """ + PendingSecondaryEmailChange.objects.create( + user=self.test_user, + new_secondary_email='pending-secondary@example.com', + activation_key='b' * 32, + ) + assert len(PendingSecondaryEmailChange.objects.filter(user_id=self.test_user.id)) == 1 + + self.client.login(username=self.test_user.username, password=self.test_password) + headers = build_jwt_headers(self.test_user) + response = self.client.post(self.url, self.build_post(self.test_password), **headers) + assert response.status_code == status.HTTP_204_NO_CONTENT + + assert len(PendingSecondaryEmailChange.objects.filter(user_id=self.test_user.id)) == 0 + def test_password_mismatch(self): """ Verify that the user submitting a mismatched password results in @@ -1293,6 +1312,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') @@ -1399,6 +1430,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() diff --git a/openedx/core/djangoapps/user_api/accounts/utils.py b/openedx/core/djangoapps/user_api/accounts/utils.py index ab70b8f15c8d..c17d1d203b1c 100644 --- a/openedx/core/djangoapps/user_api/accounts/utils.py +++ b/openedx/core/djangoapps/user_api/accounts/utils.py @@ -15,7 +15,12 @@ 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, + 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 @@ -219,6 +224,7 @@ def create_retirement_request_and_deactivate_account(user): # Delete OAuth tokens associated with the user. retire_dot_oauth2_models(user) AccountRecovery.retire_recovery_email(user.id) + PendingSecondaryEmailChange.redact_pending_secondary_email(user.id) def username_suffix_generator(suffix_length=4): diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 90d7ba70cb86..7f18fdf94def 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -37,11 +37,13 @@ from common.djangoapps.entitlements.models import CourseEntitlement 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, @@ -1152,8 +1154,15 @@ 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.delete_by_user_value(user, field="user") UserOrgTag.delete_by_user_value(user, field="user") + PendingSecondaryEmailChange.redact_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") @@ -1171,6 +1180,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) diff --git a/openedx/core/djangoapps/user_api/management/commands/retire_user.py b/openedx/core/djangoapps/user_api/management/commands/retire_user.py index 1b3b497ea5c7..47a4a8ac611b 100644 --- a/openedx/core/djangoapps/user_api/management/commands/retire_user.py +++ b/openedx/core/djangoapps/user_api/management/commands/retire_user.py @@ -7,7 +7,12 @@ from django.db import transaction 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, + PendingSecondaryEmailChange, + Registration, + get_retired_email_by_email, +) from openedx.core.djangolib.oauth2_retirement_utils import retire_dot_oauth2_models from ...models import BulkUserRetirementConfig, UserRetirementStatus @@ -158,6 +163,7 @@ def handle(self, *args, **options): # Delete OAuth tokens associated with the user. retire_dot_oauth2_models(user) AccountRecovery.retire_recovery_email(user.id) + PendingSecondaryEmailChange.redact_pending_secondary_email(user.id) except KeyError: error_message = f'Username not specified {user}' logger.error(error_message) diff --git a/openedx/core/djangoapps/user_api/management/tests/test_retire_user.py b/openedx/core/djangoapps/user_api/management/tests/test_retire_user.py index 6de2287fc4ba..f7378f0db694 100644 --- a/openedx/core/djangoapps/user_api/management/tests/test_retire_user.py +++ b/openedx/core/djangoapps/user_api/management/tests/test_retire_user.py @@ -10,6 +10,7 @@ from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core.management import CommandError, call_command +from common.djangoapps.student.models import PendingSecondaryEmailChange from common.djangoapps.student.tests.factories import UserFactory # lint-amnesty, pylint: disable=wrong-import-order from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import ( # lint-amnesty, pylint: disable=unused-import, wrong-import-order setup_retirement_states, # noqa: F401 @@ -107,3 +108,18 @@ def test_retire_with_username_email_userfile(setup_retirement_states): # lint-a with pytest.raises(CommandError, match=r'You cannot use userfile option with username and user_email'): call_command('retire_user', user_file=user_file, username=username, user_email=user_email) remove_user_file() + + +@skip_unless_lms +def test_retire_user_cleans_pending_secondary_email(setup_retirement_states): # lint-amnesty, pylint: disable=redefined-outer-name, unused-argument + user = UserFactory.create(username='user-cleanup', email='user-cleanup@example.com') + PendingSecondaryEmailChange.objects.create( + user=user, + new_secondary_email='pending-secondary@example.com', + activation_key='c' * 32, + ) + assert PendingSecondaryEmailChange.objects.filter(user=user).exists() + + call_command('retire_user', username=user.username, user_email=user.email) + + assert not PendingSecondaryEmailChange.objects.filter(user=user).exists() From da2bbfd8fb995cc8d52c77a6dd779d63d9e181f2 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Thu, 23 Apr 2026 11:05:12 +0000 Subject: [PATCH 2/3] fix: retirement PII leaks by redacting pending secondary email/name data --- .../djangoapps/user_api/management/tests/test_retire_user.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/user_api/management/tests/test_retire_user.py b/openedx/core/djangoapps/user_api/management/tests/test_retire_user.py index f7378f0db694..1d29c46b3388 100644 --- a/openedx/core/djangoapps/user_api/management/tests/test_retire_user.py +++ b/openedx/core/djangoapps/user_api/management/tests/test_retire_user.py @@ -111,7 +111,7 @@ def test_retire_with_username_email_userfile(setup_retirement_states): # lint-a @skip_unless_lms -def test_retire_user_cleans_pending_secondary_email(setup_retirement_states): # lint-amnesty, pylint: disable=redefined-outer-name, unused-argument +def test_retire_user_cleans_pending_secondary_email(setup_retirement_states): # lint-amnesty, pylint: disable=redefined-outer-name, unused-argument # noqa: F811 user = UserFactory.create(username='user-cleanup', email='user-cleanup@example.com') PendingSecondaryEmailChange.objects.create( user=user, From 4326d693f088d133920d6e207d6032f1dbfbd881 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Mon, 27 Apr 2026 05:55:25 +0000 Subject: [PATCH 3/3] fix: retirement PII leaks by redacting pending secondary email/name data --- common/djangoapps/student/models/user.py | 12 +++++++----- common/djangoapps/student/tests/test_models.py | 8 ++++---- openedx/core/djangoapps/user_api/accounts/utils.py | 2 +- openedx/core/djangoapps/user_api/accounts/views.py | 2 +- .../user_api/management/commands/retire_user.py | 2 +- 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/common/djangoapps/student/models/user.py b/common/djangoapps/student/models/user.py index da3032a66920..423c86e656cc 100644 --- a/common/djangoapps/student/models/user.py +++ b/common/djangoapps/student/models/user.py @@ -943,9 +943,9 @@ class PendingSecondaryEmailChange(DeletableByUserValue, models.Model): # noqa: activation_key = models.CharField(('activation key'), max_length=32, unique=True, db_index=True) @classmethod - def redact_pending_secondary_email(cls, user_id): + def redact_and_delete_pending_secondary_email(cls, user_id): """ - Redact a pending secondary email change row for a user. + 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. @@ -1738,14 +1738,16 @@ def update_recovery_email(self, email): @classmethod def retire_recovery_email(cls, user_id): """ - Redact user's recovery/secondary email as part of GDPR Phase I. - Returns 'True' + Redact and delete user's recovery/secondary email as part of GDPR Phase I. 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. + :param user_id: int - :return: bool + :return: bool - True on success """ try: account_recovery = cls.objects.get(user_id=user_id) diff --git a/common/djangoapps/student/tests/test_models.py b/common/djangoapps/student/tests/test_models.py index c5ce4bd55a1e..3a565bcf3c36 100644 --- a/common/djangoapps/student/tests/test_models.py +++ b/common/djangoapps/student/tests/test_models.py @@ -751,7 +751,7 @@ def test_retire_recovery_email(self): class TestPendingSecondaryEmailChange(TestCase): """Tests for retiring PendingSecondaryEmailChange records.""" - def test_redact_pending_secondary_email(self): + def test_redact_and_delete_pending_secondary_email(self): """Assert that pending secondary email records are deleted for retired users.""" user = UserFactory() PendingSecondaryEmailChange.objects.create( @@ -760,13 +760,13 @@ def test_redact_pending_secondary_email(self): activation_key='a' * 32, ) assert len(PendingSecondaryEmailChange.objects.filter(user_id=user.id)) == 1 - PendingSecondaryEmailChange.redact_pending_secondary_email(user_id=user.id) + PendingSecondaryEmailChange.redact_and_delete_pending_secondary_email(user_id=user.id) assert len(PendingSecondaryEmailChange.objects.filter(user_id=user.id)) == 0 - def test_redact_pending_secondary_email_when_no_record(self): + def test_redact_and_delete_pending_secondary_email_when_no_record(self): """Assert retirement cleanup returns True when no pending secondary row exists.""" user = UserFactory() - assert PendingSecondaryEmailChange.redact_pending_secondary_email(user_id=user.id) is True + assert PendingSecondaryEmailChange.redact_and_delete_pending_secondary_email(user_id=user.id) is True @ddt.ddt diff --git a/openedx/core/djangoapps/user_api/accounts/utils.py b/openedx/core/djangoapps/user_api/accounts/utils.py index c17d1d203b1c..6b5448eeefab 100644 --- a/openedx/core/djangoapps/user_api/accounts/utils.py +++ b/openedx/core/djangoapps/user_api/accounts/utils.py @@ -224,7 +224,7 @@ def create_retirement_request_and_deactivate_account(user): # Delete OAuth tokens associated with the user. retire_dot_oauth2_models(user) AccountRecovery.retire_recovery_email(user.id) - PendingSecondaryEmailChange.redact_pending_secondary_email(user.id) + PendingSecondaryEmailChange.redact_and_delete_pending_secondary_email(user.id) def username_suffix_generator(suffix_length=4): diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 7f18fdf94def..21131fbc7539 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1161,7 +1161,7 @@ def post(self, request): pending_email.save(update_fields=['new_email']) PendingEmailChange.delete_by_user_value(user, field="user") UserOrgTag.delete_by_user_value(user, field="user") - PendingSecondaryEmailChange.redact_pending_secondary_email(user.id) + 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 diff --git a/openedx/core/djangoapps/user_api/management/commands/retire_user.py b/openedx/core/djangoapps/user_api/management/commands/retire_user.py index 47a4a8ac611b..8ab371a1fe70 100644 --- a/openedx/core/djangoapps/user_api/management/commands/retire_user.py +++ b/openedx/core/djangoapps/user_api/management/commands/retire_user.py @@ -163,7 +163,7 @@ def handle(self, *args, **options): # Delete OAuth tokens associated with the user. retire_dot_oauth2_models(user) AccountRecovery.retire_recovery_email(user.id) - PendingSecondaryEmailChange.redact_pending_secondary_email(user.id) + PendingSecondaryEmailChange.redact_and_delete_pending_secondary_email(user.id) except KeyError: error_message = f'Username not specified {user}' logger.error(error_message)