diff --git a/common/djangoapps/student/models/user.py b/common/djangoapps/student/models/user.py index c58a80eab312..bd4761956ac7 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,24 @@ 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 delete_by_user_value(cls, value, field): + """ + Redacts new_email then deletes records where ``field`` equals ``value``. + + Returns True if deleted, False if no matching records found. + """ + filter_kwargs = {field: value} + records_matching_user_value = cls.objects.filter(**filter_kwargs) + + if not records_matching_user_value.exists(): + return False + + # Redact new_email before deletion using bulk update + records_matching_user_value.update(new_email='redacted@retired.invalid') + records_matching_user_value.delete() + return True + def request_change(self, email): """Request a change to a user's email. diff --git a/common/djangoapps/student/tests/test_email.py b/common/djangoapps/student/tests/test_email.py index 467eae934e56..97ec8b069225 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 # 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,33 @@ 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 + + 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@retired.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 58e89bde285b..08153ce390e2 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 # 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 @@ -600,6 +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_delete_by_user_value_redacts_pending_email_before_deletion(self): + """ + Verify that delete_by_user_value redacts new_email before deletion. + """ + 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'] == 'redacted@retired.invalid' + 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): # pylint: disable=missing-class-docstring diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 16f6d614a1ac..5272aaa9ef2f 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -963,14 +963,17 @@ def confirm_email_change(request, key): user.email = pec.new_email user.save() - pec.delete() + + # Redacts pending email before deletion + 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, user.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': user.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 9ba30376c55c..21dba90d25a6 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 @@ -13,6 +13,7 @@ from django.core import mail 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 @@ -1510,6 +1511,34 @@ def test_retire_user_twice_idempotent(self): fake_completed_retirement(self.test_user) self.post_and_assert_status(data) + def test_retire_user_redacts_pending_email_before_delete(self): + """ + Verify that delete_by_user_value redacts new_email using bulk update before deletion. + """ + captured_state = {} + + def capture_before_delete(sender, instance, **kwargs): + """Capture email value before it's deleted.""" + captured_state['new_email'] = instance.new_email + + # 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['new_email'] == 'redacted@retired.invalid' + + # 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 c6515390fed1..f1214b95c95c 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/openedx/core/djangoapps/user_api/accounts/views.py @@ -1046,18 +1046,13 @@ def cleanup(self, request): if len(usernames) != len(retirements): raise UserRetirementStatus.DoesNotExist("Not all usernames exist in the COMPLETE state.") - # Redact PII fields first, then delete. In case an ETL tool is syncing data - # to a downstream data warehouse, and treats the deletes as soft-deletes, - # the data will have first been redacted, protecting the sensitive PII. - # Get the IDs of the retirements to update/delete + # Redact PII first, then delete. Protects against ETL soft-delete scenarios. retirement_ids = list(retirements.values_list('id', flat=True)) - # Update by IDs UserRetirementStatus.objects.filter(id__in=retirement_ids).update( original_username=redacted_username, original_email=redacted_email, original_name=redacted_name ) - # Delete by IDs UserRetirementStatus.objects.filter(id__in=retirement_ids, current_state=complete_state).delete() return Response(status=status.HTTP_204_NO_CONTENT) @@ -1170,7 +1165,7 @@ def post(self, request): self.retire_entitlement_support_detail(user) - # Retire misc. models that may contain PII of this user + # Retire misc. PII models. PendingEmailChange redacts before deletion for downstream safety. PendingEmailChange.delete_by_user_value(user, field="user") UserOrgTag.delete_by_user_value(user, field="user")