diff --git a/common/djangoapps/student/models/user.py b/common/djangoapps/student/models/user.py index 159c784a28eb..0631edd3d5d6 100644 --- a/common/djangoapps/student/models/user.py +++ b/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 diff --git a/common/djangoapps/student/tests/test_models.py b/common/djangoapps/student/tests/test_models.py index c4650578b32e..69e6d3916e3d 100644 --- a/common/djangoapps/student/tests/test_models.py +++ b/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): diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 2f1d7bd535e5..5609afd49ddc 100644 --- a/common/djangoapps/student/views/management.py +++ b/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") 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 35c8914117e6..657958fee63d 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 @@ -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() diff --git a/openedx/core/djangoapps/user_api/accounts/utils.py b/openedx/core/djangoapps/user_api/accounts/utils.py index 083f45ecc9e0..c5c568219d48 100644 --- a/openedx/core/djangoapps/user_api/accounts/utils.py +++ b/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): """ diff --git a/openedx/core/djangoapps/user_api/accounts/views.py b/openedx/core/djangoapps/user_api/accounts/views.py index c6515390fed1..3c873362254c 100644 --- a/openedx/core/djangoapps/user_api/accounts/views.py +++ b/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) 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 3ce738b149c0..e6577e1c5960 100644 --- a/openedx/core/djangoapps/user_api/management/commands/retire_user.py +++ b/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) 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 e57c1c50c938..87d389ac3a2e 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 @@ -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 [