Skip to content

fix: redacting user retirement data in lms #37886

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ktyagiapphelix2u wants to merge 11 commits into
base: master
Choose a base branch
from
+92 −6
Open

fix: redacting user retirement data in lms #37886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
19fc427
fix: redacting user retirement data in lms
ktyagiapphelix2u Jan 14, 2026
5ac51b6
fix: redacting user retirement data in lms
ktyagiapphelix2u Jan 21, 2026
b0e4bce
fix: redacting user retirement data in lms
ktyagiapphelix2u Jan 27, 2026
3c338d4
fix: redacting user retirement data in lms
ktyagiapphelix2u Jan 28, 2026
5972c46
fix: redacting user retirement data in lms
ktyagiapphelix2u Jan 28, 2026
83e52e1
fix: redacting user retirement data in lms
ktyagiapphelix2u Jan 29, 2026
6a33726
fix: redacting user retirement data in lms
ktyagiapphelix2u Jan 30, 2026
74f4aaa
fix: redacting user retirement data in lms
ktyagiapphelix2u Jan 30, 2026
0f4e143
fix: redacting user retirement data in lms
ktyagiapphelix2u Feb 2, 2026
23cac02
fix: redacting user retirement data in lms
ktyagiapphelix2u Feb 2, 2026
0f17086
Merge branch 'master' into ktyagi/redaction
ktyagiapphelix2u Feb 2, 2026
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
77 changes: 74 additions & 3 deletions openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
from django.contrib.sites.models import Site
from django.core import mail
from django.core.cache import cache
from django.db import connection
from django.test import TestCase
from django.test.utils import CaptureQueriesContext
from django.urls import reverse
from enterprise.models import (
EnterpriseCourseEnrollment,
Expand Down Expand Up @@ -1078,9 +1080,78 @@ def cleanup_and_assert_status(self, data=None, expected_status=status.HTTP_204_N
assert response.status_code == expected_status
return response

def test_simple_success(self):
self.cleanup_and_assert_status()
assert not UserRetirementStatus.objects.all()
def _assert_redacted_update_delete_queries(self, queries, redacted_username, redacted_email, redacted_name):
"""
Helper method to verify UPDATE and DELETE queries contain correct field-value assignments.

Args:
queries: List of captured query dicts from CaptureQueriesContext
redacted_username: Expected redacted username value
redacted_email: Expected redacted email value
redacted_name: Expected redacted name value
"""
update_queries = [q for q in queries if 'UPDATE' in q['sql'] and 'user_api_userretirementstatus' in q['sql']]
delete_queries = [q for q in queries if 'DELETE' in q['sql'] and 'user_api_userretirementstatus' in q['sql']]

# Should have 9 UPDATE and 9 DELETE queries
assert len(update_queries) == 9, f"Expected 9 UPDATE queries, found {len(update_queries)}"
assert len(delete_queries) == 9, f"Expected 9 DELETE queries, found {len(delete_queries)}"

# Verify UPDATE queries contain the correct field-value assignments
for update_query in update_queries:
sql_lower = update_query['sql']
# Check that the correct field is set with the correct value
# This ensures that if someone swaps the assignments, the test will fail
assert "original_username" in sql_lower and f"= '{redacted_username}'" in sql_lower, (
f"UPDATE query missing 'original_username = {redacted_username}': {sql_lower}"
)
assert "original_email" in sql_lower and f"= '{redacted_email}'" in sql_lower, (
f"UPDATE query missing 'original_email = {redacted_email}': {sql_lower}"
)
assert "original_name" in sql_lower and f"= '{redacted_name}'" in sql_lower, (
f"UPDATE query missing 'original_name = {redacted_name}': {sql_lower}"
)

def test_default_redacted_values(self):
"""
Test basic cleanup with default redacted values.
Verify that redaction (UPDATE) happens before deletion (DELETE).
Captures actual SQL queries to ensure UPDATE queries contain correct field-value assignments.
"""
with CaptureQueriesContext(connection) as context:
self.cleanup_and_assert_status()

# Verify records are deleted after redaction
retirements = UserRetirementStatus.objects.all()
assert retirements.count() == 0

# Verify UPDATE and DELETE queries with default 'redacted' value
self._assert_redacted_update_delete_queries(context.captured_queries, 'redacted', 'redacted', 'redacted')

def test_custom_redacted_values(self):
"""Test that custom redacted values are applied before deletion."""
custom_username = 'username-redacted-12345'
custom_email = 'email-redacted-67890'
custom_name = 'name-redacted-abcde'

data = {
'usernames': self.usernames,
'redacted_username': custom_username,
'redacted_email': custom_email,
'redacted_name': custom_name
}

with CaptureQueriesContext(connection) as context:
self.cleanup_and_assert_status(data=data)

# Verify records are deleted after redaction
retirements = UserRetirementStatus.objects.all()
assert retirements.count() == 0

# Verify UPDATE and DELETE queries with custom redacted values
self._assert_redacted_update_delete_queries(
context.captured_queries, custom_username, custom_email, custom_name
)

def test_leaves_other_users(self):
remaining_usernames = []
Expand Down
21 changes: 18 additions & 3 deletions openedx/core/djangoapps/user_api/accounts/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1024,14 +1024,20 @@ def cleanup(self, request):

```
{
'usernames': ['user1', 'user2', ...]
'usernames': ['user1', 'user2', ...],
'redacted_username': 'Value to store in username field',
'redacted_email': 'Value to store in email field',
'redacted_name': 'Value to store in name field'
}
```

Deletes a batch of retirement requests by username.
Redacts a batch of retirement requests by redacting PII fields.
"""
try:
usernames = request.data["usernames"]
redacted_username = request.data.get("redacted_username", "redacted")
redacted_email = request.data.get("redacted_email", "redacted")
redacted_name = request.data.get("redacted_name", "redacted")

if not isinstance(usernames, list):
raise TypeError("Usernames should be an array.")
Expand All @@ -1045,7 +1051,16 @@ def cleanup(self, request):
if len(usernames) != len(retirements):
raise UserRetirementStatus.DoesNotExist("Not all usernames exist in the COMPLETE state.")

retirements.delete()
# 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.
for retirement in retirements:
retirement.original_username = redacted_username
retirement.original_email = redacted_email
retirement.original_name = redacted_name
retirement.save()
retirement.delete()
Comment on lines +1054 to +1062
Copy link
Contributor

@robrap robrap Feb 4, 2026

Choose a reason for hiding this comment

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

@ktyagiapphelix2u: I thought you had said this work was complete. I guess I misunderstood. Either way, please take care of this and all copilot comments. Thank you.

Copy link
Contributor

@robrap robrap Feb 4, 2026

Choose a reason for hiding this comment

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

Oh. Is this a very outdated duplicate PR for openedx? We should discuss our process, but I would have imagined that this would be the PR we start with.

Copy link
Contributor Author

@ktyagiapphelix2u ktyagiapphelix2u Feb 5, 2026

Choose a reason for hiding this comment

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

@robrap You are seeing the old PR, the new PR is in edx/edx-platform. Thanks.

Copy link
Contributor

@robrap robrap Feb 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

In general, let's work in openedx first. For this, let's wrap up the review in edx#105, and you can squash *when done on the other branch) and use it to update this branch, so we can have the reviewers get a chance to see this again since it has been updated. Thank you.


return Response(status=status.HTTP_204_NO_CONTENT)
except (RetirementStateError, UserRetirementStatus.DoesNotExist, TypeError) as exc:
return Response(str(exc), status=status.HTTP_400_BAD_REQUEST)
Expand Down
Loading