Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion common/djangoapps/student/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -904,14 +904,33 @@ 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
"""
user = models.OneToOneField(User, unique=True, db_index=True, on_delete=models.CASCADE)
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.

Returns True if redacted, and False if no matching records found.
"""
Comment on lines +918 to +924
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Docstrings should have a one line summary, and an optional blank line and longer description. Like a comment message.
  • Also, maybe add something like:
        Returns True if redacted, and 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
for record in records_matching_user_value:
Comment on lines +926 to +929
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both queries fetch the same data. Since the queryset is lazy, .exists() and the for loop each trigger a separate SQL query. Change to:

Suggested change
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:
records = list(cls.objects.filter(**filter_kwargs))
if not records:
return False
for record in records:

This is a single DB hit, which matters more if the field filter ever doesn't use the OneToOneField on user.

Note: The change is optional.

record.new_email = get_retired_email_by_email(record.new_email)
record.save(update_fields=['new_email'])
Comment on lines +930 to +931
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description explicitly identifies activation_key as sensitive data that can still persist indirectly in logs, backups, or downstream systems. However, the implementation only redacts new_email . activation_key is left as-is. If downstream systems snapshot these rows, the activation key still leaks. It should be cleared before deletion, e.g.:

Suggested change
record.new_email = get_retired_email_by_email(record.new_email)
record.save(update_fields=['new_email'])
record.new_email = get_retired_email_by_email(record.new_email)
record.activation_key = '' # or a redacted value
record.save(update_fields=['new_email', 'activation_key'])

return True

def request_change(self, email):
"""Request a change to a user's email.

Expand Down
16 changes: 16 additions & 0 deletions common/djangoapps/student/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -600,6 +601,21 @@ 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):
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be updated to test for multiple pending records, and ensuring that they are all redacted?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user field is a OneToOneField with unique=True, so there can only be one PendingEmailChange per user. The test already covers the maximum case (redacting 1 record). Multiple records per user aren't possible with this model constraint.

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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If activation_key redaction is added, this assertion must be updated to verify the key is also cleared/replaced. As-is, this test will need to change regardless once above issue is fixed.


def test_redact_by_user_no_effect_for_user_with_no_email_change(self):
"""Verify that redacting a user with no pending email change returns False."""
record_was_redacted = PendingEmailChange.redact_pending_email_by_user_value(self.user2, field='user')
assert not record_was_redacted
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand this test. Should it just have lines 617 and 618, where you ask to redact on a user that isn't in the table, and it returns that it didn't redact?

All the other details about the user 1 email change seem irrelevant and confusing. If you think it is important, I'd need better comments.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test now:

  1. Has a clear docstring explaining it verifies redacting a user with no pending email change returns False
  2. Only tests the relevant behavior - calling redact on user2 (who has no email change record) returns False
  3. Removes the confusing assertions about user1's email change data remaining unchanged



class TestCourseEnrollmentAllowed(ModuleStoreTestCase): # lint-amnesty, pylint: disable=missing-class-docstring

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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()

Expand All @@ -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):
"""
Expand Down
5 changes: 4 additions & 1 deletion openedx/core/djangoapps/user_api/accounts/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down
Loading