From 5e6a070720ab058f8052ee83e7fb045349387681 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Thu, 23 Apr 2026 10:15:37 +0000 Subject: [PATCH 1/3] fix: redact pending primary email before retirement deletion --- common/djangoapps/student/models/user.py | 18 ++++++++++++- .../djangoapps/student/tests/test_models.py | 20 ++++++++++++++ .../accounts/tests/test_retirement_views.py | 26 ++++++++++++++++++- .../djangoapps/user_api/accounts/views.py | 5 +++- 4 files changed, 66 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/student/models/user.py b/common/djangoapps/student/models/user.py index 6b7cdb7e761a..e235908acbde 100644 --- a/common/djangoapps/student/models/user.py +++ b/common/djangoapps/student/models/user.py @@ -904,7 +904,7 @@ 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 """ @@ -912,6 +912,22 @@ class PendingEmailChange(DeletableByUserValue, models.Model): # noqa: DJ008 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 redact_pending_email_by_user_value(cls, value, field): + """ + Redact pending email change fields for records matching ``field=value``. + This method is intended for retirement flows where downstream systems + may keep soft-deleted snapshots of these rows. + """ + filter_kwargs = {field: value} + records_matching_user_value = cls.objects.filter(**filter_kwargs) + if not records_matching_user_value.exists(): + return False + for record in records_matching_user_value: + record.new_email = get_retired_email_by_email(record.new_email) + record.save(update_fields=['new_email']) + return True + def request_change(self, email): """Request a change to a user's email. diff --git a/common/djangoapps/student/tests/test_models.py b/common/djangoapps/student/tests/test_models.py index 09a1d35fa424..35daa83f5f1b 100644 --- a/common/djangoapps/student/tests/test_models.py +++ b/common/djangoapps/student/tests/test_models.py @@ -30,6 +30,7 @@ UserAttribute, UserCelebration, UserProfile, + get_retired_email_by_email, ) from common.djangoapps.student.models_api import confirm_name_change, do_name_change_request, get_name from common.djangoapps.student.tests.factories import AccountRecoveryFactory, CourseEnrollmentFactory, UserFactory @@ -600,6 +601,25 @@ 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_redact_by_user_redacts_pending_email_change_fields(self): + original_new_email = self.email_change.new_email + original_activation_key = self.email_change.activation_key + expected_retired_email = get_retired_email_by_email(original_new_email) + record_was_redacted = PendingEmailChange.redact_pending_email_by_user_value(self.user, field='user') + assert record_was_redacted + self.email_change.refresh_from_db() + assert self.email_change.new_email == expected_retired_email + assert self.email_change.activation_key == original_activation_key + + def test_redact_by_user_no_effect_for_user_with_no_email_change(self): + original_new_email = self.email_change.new_email + original_activation_key = self.email_change.activation_key + record_was_redacted = PendingEmailChange.redact_pending_email_by_user_value(self.user2, field='user') + assert not record_was_redacted + self.email_change.refresh_from_db() + assert self.email_change.new_email == original_new_email + assert self.email_change.activation_key == original_activation_key + class TestCourseEnrollmentAllowed(ModuleStoreTestCase): # lint-amnesty, pylint: disable=missing-class-docstring 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..89293dd4d7b7 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 @@ -1365,7 +1365,13 @@ def test_retire_user_where_username_not_provided(self): @mock.patch('openedx.core.djangoapps.user_api.accounts.views.get_profile_image_names') @mock.patch('openedx.core.djangoapps.user_api.accounts.views.remove_profile_images') - def test_retire_user(self, mock_remove_profile_images, mock_get_profile_image_names): + @mock.patch('openedx.core.djangoapps.user_api.accounts.views.PendingEmailChange.redact_pending_email_by_user_value') + def test_retire_user( + self, + mock_redact_pending_email, + mock_remove_profile_images, + mock_get_profile_image_names, + ): data = {'username': self.original_username} self.post_and_assert_status(data) @@ -1396,6 +1402,7 @@ def test_retire_user(self, mock_remove_profile_images, mock_get_profile_image_na self._entitlement_support_detail_assertions() + mock_redact_pending_email.assert_called_once_with(self.test_user, field="user") assert not PendingEmailChange.objects.filter(user=self.test_user).exists() assert not UserOrgTag.objects.filter(user=self.test_user).exists() @@ -1408,6 +1415,23 @@ def test_retire_user_twice_idempotent(self): fake_completed_retirement(self.test_user) self.post_and_assert_status(data) + @mock.patch('openedx.core.djangoapps.user_api.accounts.views.PendingEmailChange.delete_by_user_value') + def test_retire_user_redacts_pending_email_before_delete(self, mock_delete_pending_email): + pending_email_record = PendingEmailChange.objects.get(user=self.test_user) + pending_email_before_retirement = pending_email_record.new_email + expected_retired_pending_email = get_retired_email_by_email(pending_email_before_retirement) + + def _assert_redacted_then_delete(value, field): + pending_record = PendingEmailChange.objects.get(user=self.test_user) + assert pending_record.new_email == expected_retired_pending_email + pending_record.delete() + return True + + mock_delete_pending_email.side_effect = _assert_redacted_then_delete + data = {'username': self.original_username} + self.post_and_assert_status(data) + assert not PendingEmailChange.objects.filter(user=self.test_user).exists() + @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): """ diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 90d7ba70cb86..2e1a994ed1cb 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1151,7 +1151,10 @@ def post(self, request): self.retire_entitlement_support_detail(user) - # Retire misc. models that may contain PII of this user + # Retire misc. models that may contain PII of this user. + # Redact pending email before delete because downstream systems + # may preserve soft-deleted snapshots. + 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") From 07bf642bd988d5e25309c2e7474737a67e6dafdf Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Mon, 4 May 2026 09:30:48 +0000 Subject: [PATCH 2/3] fix: redact pending primary email before retirement deletion --- common/djangoapps/student/models/user.py | 20 +++++--- common/djangoapps/student/tests/test_email.py | 29 ++++++++++++ .../djangoapps/student/tests/test_models.py | 47 +++++++++++-------- common/djangoapps/student/views/management.py | 12 +++-- .../accounts/tests/test_retirement_views.py | 46 +++++++++++------- .../djangoapps/user_api/accounts/views.py | 5 +- 6 files changed, 108 insertions(+), 51 deletions(-) diff --git a/common/djangoapps/student/models/user.py b/common/djangoapps/student/models/user.py index e235908acbde..1c6f826aa5c0 100644 --- a/common/djangoapps/student/models/user.py +++ b/common/djangoapps/student/models/user.py @@ -913,19 +913,25 @@ class PendingEmailChange(DeletableByUserValue, models.Model): # noqa: DJ008 activation_key = models.CharField(('activation key'), max_length=32, unique=True, db_index=True) @classmethod - def redact_pending_email_by_user_value(cls, value, field): + def delete_by_user_value(cls, value, field): """ - Redact pending email change fields for records matching ``field=value``. - This method is intended for retirement flows where downstream systems - may keep soft-deleted snapshots of these rows. + Deletes instances of this model where ``field`` equals ``value``. + + Automatically redacts new_email before deletion to ensure PII is cleared. + Uses bulk ORM update for efficiency. + + Returns True if any instances were deleted. + Returns False otherwise. """ filter_kwargs = {field: value} records_matching_user_value = cls.objects.filter(**filter_kwargs) + if not records_matching_user_value.exists(): return False - for record in records_matching_user_value: - record.new_email = get_retired_email_by_email(record.new_email) - record.save(update_fields=['new_email']) + + # Redact new_email before deletion using bulk update + records_matching_user_value.update(new_email='redacted@redacted.invalid') + records_matching_user_value.delete() return True def request_change(self, email): diff --git a/common/djangoapps/student/tests/test_email.py b/common/djangoapps/student/tests/test_email.py index d9aa2dd19662..de82ae4be2ab 100644 --- a/common/djangoapps/student/tests/test_email.py +++ b/common/djangoapps/student/tests/test_email.py @@ -10,6 +10,7 @@ from django.contrib.auth.models import User # lint-amnesty, 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,34 @@ 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 + + ace_mail.send.side_effect = [None, None] + 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@redacted.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() diff --git a/common/djangoapps/student/tests/test_models.py b/common/djangoapps/student/tests/test_models.py index 35daa83f5f1b..e49bfe9387f9 100644 --- a/common/djangoapps/student/tests/test_models.py +++ b/common/djangoapps/student/tests/test_models.py @@ -11,6 +11,7 @@ from django.contrib.auth.models import AnonymousUser, User # lint-amnesty, 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 @@ -30,7 +31,6 @@ UserAttribute, UserCelebration, UserProfile, - get_retired_email_by_email, ) from common.djangoapps.student.models_api import confirm_name_change, do_name_change_request, get_name from common.djangoapps.student.tests.factories import AccountRecoveryFactory, CourseEnrollmentFactory, UserFactory @@ -601,24 +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_redact_by_user_redacts_pending_email_change_fields(self): - original_new_email = self.email_change.new_email - original_activation_key = self.email_change.activation_key - expected_retired_email = get_retired_email_by_email(original_new_email) - record_was_redacted = PendingEmailChange.redact_pending_email_by_user_value(self.user, field='user') - assert record_was_redacted - self.email_change.refresh_from_db() - assert self.email_change.new_email == expected_retired_email - assert self.email_change.activation_key == original_activation_key - - def test_redact_by_user_no_effect_for_user_with_no_email_change(self): - original_new_email = self.email_change.new_email - original_activation_key = self.email_change.activation_key - record_was_redacted = PendingEmailChange.redact_pending_email_by_user_value(self.user2, field='user') - assert not record_was_redacted - self.email_change.refresh_from_db() - assert self.email_change.new_email == original_new_email - assert self.email_change.activation_key == original_activation_key + def test_delete_by_user_value_redacts_pending_email_before_deletion(self): + """ + Verify that delete_by_user_value redacts new_email before deletion. + """ + expected_redacted_email = 'redacted@redacted.invalid' + 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'] == expected_redacted_email + 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): # lint-amnesty, pylint: disable=missing-class-docstring diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index cf7948b448bb..e23b13c320dd 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -961,16 +961,20 @@ def confirm_email_change(request, key): transaction.set_rollback(True) return response - user.email = pec.new_email + new_email = pec.new_email + user.email = new_email user.save() - pec.delete() + + # Use model-level deletion to redact pending email before delete. + 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, new_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': new_email}) transaction.set_rollback(True) return response 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 89293dd4d7b7..28e9103facea 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 @@ -10,8 +10,9 @@ import ddt from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.contrib.sites.models import Site -from django.core import mail from django.core.cache import cache +from django.core import mail +from django.db.models.signals import pre_delete from django.test import TestCase from django.urls import reverse from opaque_keys.edx.keys import CourseKey @@ -1365,10 +1366,8 @@ def test_retire_user_where_username_not_provided(self): @mock.patch('openedx.core.djangoapps.user_api.accounts.views.get_profile_image_names') @mock.patch('openedx.core.djangoapps.user_api.accounts.views.remove_profile_images') - @mock.patch('openedx.core.djangoapps.user_api.accounts.views.PendingEmailChange.redact_pending_email_by_user_value') def test_retire_user( self, - mock_redact_pending_email, mock_remove_profile_images, mock_get_profile_image_names, ): @@ -1402,7 +1401,6 @@ def test_retire_user( self._entitlement_support_detail_assertions() - mock_redact_pending_email.assert_called_once_with(self.test_user, field="user") assert not PendingEmailChange.objects.filter(user=self.test_user).exists() assert not UserOrgTag.objects.filter(user=self.test_user).exists() @@ -1415,22 +1413,34 @@ def test_retire_user_twice_idempotent(self): fake_completed_retirement(self.test_user) self.post_and_assert_status(data) - @mock.patch('openedx.core.djangoapps.user_api.accounts.views.PendingEmailChange.delete_by_user_value') - def test_retire_user_redacts_pending_email_before_delete(self, mock_delete_pending_email): - pending_email_record = PendingEmailChange.objects.get(user=self.test_user) - pending_email_before_retirement = pending_email_record.new_email - expected_retired_pending_email = get_retired_email_by_email(pending_email_before_retirement) + def test_retire_user_redacts_pending_email_before_delete(self): + """ + Verify that delete_by_user_value redacts new_email using bulk update before deletion. + """ + expected_redacted_email = 'redacted@redacted.invalid' + captured_state = {} - def _assert_redacted_then_delete(value, field): - pending_record = PendingEmailChange.objects.get(user=self.test_user) - assert pending_record.new_email == expected_retired_pending_email - pending_record.delete() - return True + def capture_before_delete(sender, instance, **kwargs): + """Capture email value before it's deleted.""" + captured_state['new_email'] = instance.new_email - mock_delete_pending_email.side_effect = _assert_redacted_then_delete - data = {'username': self.original_username} - self.post_and_assert_status(data) - assert not PendingEmailChange.objects.filter(user=self.test_user).exists() + # 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.get('new_email') == expected_redacted_email + + # 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): diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index 2e1a994ed1cb..8b525328fff9 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1152,9 +1152,8 @@ def post(self, request): self.retire_entitlement_support_detail(user) # Retire misc. models that may contain PII of this user. - # Redact pending email before delete because downstream systems - # may preserve soft-deleted snapshots. - PendingEmailChange.redact_pending_email_by_user_value(user, field="user") + # PendingEmailChange.delete_by_user_value() automatically redacts new_email + # before deletion because downstream systems may preserve soft-deleted snapshots. PendingEmailChange.delete_by_user_value(user, field="user") UserOrgTag.delete_by_user_value(user, field="user") From ee86eebb22e51ea2ea6a8ed41f7f47461c1a3c04 Mon Sep 17 00:00:00 2001 From: ktyagiapphelix2u Date: Mon, 4 May 2026 09:38:01 +0000 Subject: [PATCH 3/3] fix: redact pending primary email before retirement deletion --- .../user_api/accounts/tests/test_retirement_views.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 4324021ae019..b461c0c2c9a5 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 @@ -10,10 +10,10 @@ import ddt from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.contrib.sites.models import Site -from django.core.cache import cache from django.core import mail -from django.db.models.signals import pre_delete +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