feat: redact PII fields before delete in user retirement flows by ktyagiapphelix2u · Pull Request #38426 · openedx/openedx-platform

ktyagiapphelix2u · 2026-04-23T10:23:01Z

Summary

Adds support for redacting sensitive PII fields before deletion in DeletableByUserValue models.

Changes include:

Added redact_before_delete_fields hook to model mixin
Redact emails before delete for:
- CourseEnrollmentAllowed
- PendingEmailChange
- UnregisteredLearnerCohortAssignments
Added SQL assertion helpers to verify UPDATE occurs before DELETE
Added regression/unit tests for safe ID-filtered redaction and deletion flows
Updated email change flow to use delete_by_user_value

Ticket & Reference

https://2u-internal.atlassian.net/browse/BOMS-498

Related PR

https://2u-internal.atlassian.net/browse/BOMS-565
https://2u-internal.atlassian.net/browse/BOMS-564

vgulati-apphelix · 2026-05-06T13:22:38Z

This change addresses a privacy issue in the retirement flow for users who have a pending primary email change.

Please elaborate the problem statement and solution provided.

robrap

Thanks.

robrap

I didn't get through the review fully. Sorry. So just leaving you with what I have.

robrap

Thanks. Is looking good and getting close. We're almost there.

robrap · 2026-05-26T20:33:20Z

+    table,
+    num_redact_delete_pairs=1,
+    require_id_filter=False,
+    expected_redacted_value=None,


Is there a reason why this is optional? At least one of the tests doesn't send this. Is there a reason?

That is fair. I kept expected_redacted_value optional for helper reuse, but I updated the current PendingEmailChange tests to pass the explicit redacted value so coverage is stronger and no test relies on the default here.

I still don't understand why this is optional. This is a test for redaction. The update shouldn't be any update, but one that includes the redacted value. Is there a call where you couldn't send the redacted value?

I'm not clear on the usefulness of require_id_filter. Do we care how it is filtered? I would consider dropping this and the accompanying assertions, but maybe you could explain its importance?

agreed on both. I've made expected_redacted_value a required positional parameter there's no valid call site that couldn't provide it, so making it optional was just unnecessary flexibility.

Dropped require_id_filter entirely as well, we don't need to assert how the rows are filtered, only that the redaction UPDATE happens before the DELETE, which is what the helper already verifies.

Both issues are fixed.

expected_redacted_value is now a required positional parameter since every valid call site already provides it, so the optional default was unnecessary.

require_id_filter and its related assertions have been removed. We only need to verify that the redaction UPDATE happens before the DELETE

robrap · 2026-05-26T20:49:24Z

+        record_ids = list(records_matching_user_value.values_list('id', flat=True))
+        records_matching_ids = cls.objects.filter(id__in=record_ids)


If needed at all, shouldn't this live in if redact_fields: conditional, so it is only needed when we actually need to do an update?

What is the difference between records_matching_user_value and records_matching_ids, and why do we need to do another lookup? The original code already had records_matching_user_value.delete(), which is presumably deleting the correct records. Isn't this what we want to update?

I’ve updated the mixin so the second lookup only happens when redaction is actually needed.

records_matching_user_value is the original queryset filtered by the user field (for example email=value).

We only build the ID-based queryset when redact_fields is non-empty, because redacting may change that same filter field, and we still need to delete the exact same rows.

For non-redacting models, we now directly call delete on the original queryset (no extra lookup).

See #38426 (comment) for continuation.

robrap · 2026-05-26T20:50:58Z

+
+        @classmethod
+        def redact_before_delete_fields(cls):
+            return {'email': 'redacted-before-delete@safe.com'}


Can we adjust this to include two fields to ensure that would work appropriately?

yes updated

robrap · 2026-05-26T20:58:15Z

+        with mock.patch.object(model_cls, 'objects', create=True) as mock_objects:
+            mock_objects.filter.return_value = queryset


All this mocking makes me nervous. Can the test models be updated to be real Django models, like?:

class NonRedactingModel(DeletableByUserValue, models.Model):

Not sure how simple this would be, but the less mocking the better, especially in this case.

Thanks. but

I kept this test mocked because it is a small unit test for mixin logic only.
Making real Django models here would add a lot of setup and make the test harder to read.

We already test this behavior with real database models in:

common/djangoapps/student/tests/test_models.py openedx/core/djangoapps/course_groups/tests/test_cohorts.py openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py

So this test stays simple, and real DB behavior is already covered elsewhere.

@ktyagiapphelix2u: I think mock or non-mock would be fine, so feel free to choose what you prefer. However, I'm confused, because I thought when we met you said you liked the full Django model version better. Did you have trouble with it when you removed from INSTALLED_APPS, or something else?

robrap

I didn't get through review, but here is something...

robrap · 2026-05-27T21:55:53Z

+    table,
+    num_redact_delete_pairs=1,
+    require_id_filter=False,
+    expected_redacted_value=None,


I still don't understand why this is optional. This is a test for redaction. The update shouldn't be any update, but one that includes the redacted value. Is there a call where you couldn't send the redacted value?

I'm not clear on the usefulness of require_id_filter. Do we care how it is filtered? I would consider dropping this and the accompanying assertions, but maybe you could explain its importance?

robrap · 2026-05-27T22:18:42Z

+            # When redacting the field used for filtering, switch to ID-based
+            # operations so the subsequent delete still targets the same rows.
+            record_ids = list(records_matching_user_value.values_list('id', flat=True))
+            records_matching_ids = cls.objects.filter(id__in=record_ids)
+            records_matching_ids.update(**redact_fields)


[New comment for addressing https://github.com//pull/38426#discussion_r3309021075]

I still don't understand why this can't be records_matching_user_value.update(**redact_fields). The existing code uses records_matching_user_value.delete(), and includes all records that need to be deleted. We simply want to redact all records we are about to delete.

Does this code not work for some reason, or are you just trying to delete values outside of the scope of how this function was called? Doesn't that seem dangerous? We only want to redact what is being deleted, and we don't want to delete anything different from what this was already deleting.

@robrap I got it know that records_matching_user_value.update(**redact_fields) is the correct and simpler approach in most cases and the code now uses it by default. The ID-based path is only taken in the one specific case where it's unavoidable when the filter field is the same field being redacted (e.g. CourseEnrollmentAllowed and UnregisteredLearnerCohortAssignments both filter by email and redact email).

In that case, records_matching_user_value.update(email='redacted-before-delete@safe.com') succeeds, but the subsequent records_matching_user_value.delete() generates DELETE WHERE email = 'original@example.com' — which now matches zero rows because the email was just changed. The rows get redacted but never deleted.

Capturing IDs before the update is the minimal fix for that specific situation, and it only affects those two models. All other callers (filtering by user, not email) now use the simple path you suggested.

Updated the CODE

robrap

@ktyagiapphelix2u: It's looking great. Very minor proposals. Let's try to look at your final changes together on Monday so we can merge. Thank you.

robrap · 2026-05-29T21:27:43Z

+"""
+
+
+def assert_update_before_delete(


Let's rename this to assert_redact_before_delete.

robrap · 2026-05-29T21:30:44Z

+    Assert that UPDATE and DELETE queries for ``table`` occur in consecutive pairs,
+    and that each UPDATE contains the expected redacted value.


Proposed update:

Suggested change

Assert that UPDATE and DELETE queries for ``table`` occur in consecutive pairs,

and that each UPDATE contains the expected redacted value.

Assert that PII is redacted before deleted.

Redacting before deleting protects downstream sync of data from holding PII

in soft-deleted records. This helper ensures UPDATE and DELETE queries for

``table`` occur in consecutive pairs, and that each UPDATE contains the

expected redacted value.

robrap · 2026-05-29T21:33:54Z

@@ -0,0 +1,34 @@
+"""
+SQL assertion helpers for tests.


Let's just move this to https://github.com/openedx/openedx-platform/blob/master/openedx/core/djangolib/testing/utils.py, rather than creating a new utility file.

robrap · 2026-05-29T21:40:22Z

+        with mock.patch.object(model_cls, 'objects', create=True) as mock_objects:
+            mock_objects.filter.return_value = queryset


@ktyagiapphelix2u: I think mock or non-mock would be fine, so feel free to choose what you prefer. However, I'm confused, because I thought when we met you said you liked the full Django model version better. Did you have trouble with it when you removed from INSTALLED_APPS, or something else?

robrap · 2026-05-29T21:41:13Z

    def delete_by_user_value(cls, value, field):
        """
-        Deletes instances of this model where ``field`` equals ``value``.
+        Redacts and deletes instances of this model where ``field`` equals ``value``.


Minor update:

Suggested change

Redacts and deletes instances of this model where ``field`` equals ``value``.

Optionally redacts and always deletes instances of this model where ``field`` equals ``value``.

robrap · 2026-05-29T21:45:24Z

+        If ``redact_before_delete_fields`` returns a non-empty dict, matching
+        records are bulk-updated with those redacted values before delete.


Proposed update:

Suggested change

If ``redact_before_delete_fields`` returns a non-empty dict, matching

records are bulk-updated with those redacted values before delete.

If ``redact_before_delete_fields()`` returns a non-empty dict, the

returned PII fields are redacted before any records are deleted.

robrap · 2026-05-29T22:13:57Z

+def assert_update_before_delete(
+    sql_list,
+    table,
+    expected_redacted_value,


We might want this to be expected_redacted_value_list, and we can ensure it is non-empty, and then loop over it and check each value, in case we redact multiple values. You don't need to check that the right field is updated for each field, although we could. That would require passing in the field as well, so maybe we just keep it simple.

Note: This will allow this to be re-used for the social auth PR, I hope.

fix: redact pending primary email before retirement deletion

5e6a070

ktyagiapphelix2u marked this pull request as ready for review April 23, 2026 11:29

ktyagiapphelix2u requested a review from a team as a code owner April 23, 2026 11:29

robrap reviewed Apr 24, 2026

View reviewed changes

Comment thread common/djangoapps/student/tests/test_models.py Outdated

Comment thread common/djangoapps/student/models/user.py Outdated

Comment thread common/djangoapps/student/tests/test_models.py Outdated

Akanshu-2u reviewed Apr 27, 2026

View reviewed changes

Comment thread common/djangoapps/student/models/user.py Outdated

Akanshu-2u reviewed Apr 27, 2026

View reviewed changes

Comment thread common/djangoapps/student/tests/test_models.py Outdated

Akanshu-2u reviewed Apr 27, 2026

View reviewed changes

Comment thread common/djangoapps/student/models/user.py Outdated

Akanshu-2u mentioned this pull request Apr 28, 2026

fix: retirement PII leaks by redacting pending secondary email/name data #38427

Open

Akanshu-2u approved these changes Apr 28, 2026

View reviewed changes

robrap reviewed Apr 29, 2026

View reviewed changes

Comment thread common/djangoapps/student/models/user.py Outdated

Comment thread openedx/core/djangoapps/user_api/accounts/views.py Outdated

ktyagiapphelix2u force-pushed the ktyagi/primaryemail branch 2 times, most recently from 0d5d5e6 to 2593ac2 Compare May 4, 2026 08:49

fix: redact pending primary email before retirement deletion

07bf642

ktyagiapphelix2u force-pushed the ktyagi/primaryemail branch from f7e8036 to 07bf642 Compare May 4, 2026 09:31

ktyagiapphelix2u added 6 commits May 4, 2026 15:04

Merge branch 'master' into ktyagi/primaryemail

3f9a0d4

fix: redact pending primary email before retirement deletion

ee86eeb

fix: redact pending primary email before retirement deletion

a59c1ed

fix: redact pending primary email before retirement deletion

f6d7e93

fix: redact pending primary email before retirement deletion

33c6df3

Merge branch 'master' into ktyagi/primaryemail

3f4cffd

ktyagiapphelix2u commented May 12, 2026

View reviewed changes

Comment thread common/djangoapps/student/models/user.py Outdated

Comment thread common/djangoapps/student/models/user.py Outdated

Comment thread openedx/core/djangoapps/user_api/accounts/views.py Outdated

Comment thread common/djangoapps/student/models/user.py Outdated

ktyagiapphelix2u added 3 commits May 12, 2026 06:04

fix: redact pending primary email before retirement deletion

5165304

fix: redact pending primary email before retirement deletion

be85318

fix: redact pending primary email before retirement deletion

0d526a3

ttak-apphelix reviewed May 12, 2026

View reviewed changes

Comment thread common/djangoapps/student/tests/test_email.py Outdated

Comment thread common/djangoapps/student/tests/test_email.py Outdated

ktyagiapphelix2u added 3 commits May 12, 2026 07:34

fix: redact pending primary email before retirement deletion

af71158

fix: redact pending primary email before retirement deletion

027069b

fix: redact pending primary email before retirement deletion

568ed45

ttak-apphelix approved these changes May 12, 2026

View reviewed changes

robrap reviewed May 20, 2026

View reviewed changes

ktyagiapphelix2u added 2 commits May 20, 2026 06:28

fix: redact pending primary email before retirement deletion

d168dc4

fix: redact pending primary email before retirement deletion

03fce31

ktyagiapphelix2u force-pushed the ktyagi/primaryemail branch from 3905bd5 to 03fce31 Compare May 20, 2026 09:07

ktyagiapphelix2u added 2 commits May 21, 2026 07:03

fix: redact pending primary email before retirement deletion

4b60c74

fix: redact pending primary email before retirement deletion

36bfeb9

ktyagiapphelix2u requested a review from a team as a code owner May 21, 2026 07:37

fix: redact pending primary email before retirement deletion

ab7b642

ktyagiapphelix2u force-pushed the ktyagi/primaryemail branch from f340dae to ab7b642 Compare May 21, 2026 07:42

vgulati-apphelix reviewed May 21, 2026

View reviewed changes

Comment thread openedx/core/djangolib/model_mixins.py Outdated

vgulati-apphelix reviewed May 21, 2026

View reviewed changes

Comment thread common/djangoapps/student/tests/test_email.py Outdated

ktyagiapphelix2u added 4 commits May 22, 2026 05:43

fix: redact pending primary email before retirement deletion

6fd1b63

fix: redact pending primary email before retirement deletion

576c9e1

fix: redact pending primary email before retirement deletion

ad865a9

fix: redact pending primary email before retirement deletion

c351f22

ktyagiapphelix2u changed the title ~~fix: redact pending primary email before retirement deletion~~ feat: redact PII fields before delete in user retirement flows May 22, 2026

robrap reviewed May 22, 2026

View reviewed changes

ktyagiapphelix2u added 2 commits May 25, 2026 06:08

fix: redact pending primary email before retirement deletion

2bd85a6

fix: redact pending primary email before retirement deletion

043a69c

robrap reviewed May 26, 2026

View reviewed changes

fix: redact pending primary email before retirement deletion

ee086f5

robrap reviewed May 27, 2026

View reviewed changes

ktyagiapphelix2u added 6 commits May 28, 2026 07:47

fix: redact pending primary email before retirement deletion

fceb752

fix: redact pending primary email before retirement deletion

fea9ba3

fix: redact pending primary email before retirement deletion

35ced89

fix: redact pending primary email before retirement deletion

43f9a38

fix: redact pending primary email before retirement deletion

146b0b9

fix: redact pending primary email before retirement deletion

540258c

robrap reviewed May 29, 2026

View reviewed changes

		record_ids = list(records_matching_user_value.values_list('id', flat=True))
		records_matching_ids = cls.objects.filter(id__in=record_ids)

		with mock.patch.object(model_cls, 'objects', create=True) as mock_objects:
		mock_objects.filter.return_value = queryset

		Assert that UPDATE and DELETE queries for ``table`` occur in consecutive pairs,
		and that each UPDATE contains the expected redacted value.

-    Assert that UPDATE and DELETE queries for ``table`` occur in consecutive pairs,
-    and that each UPDATE contains the expected redacted value.
+    Assert that PII is redacted before deleted.
+    Redacting before deleting protects downstream sync of data from holding PII
+    in soft-deleted records. This helper ensures UPDATE and DELETE queries for
+    ``table`` occur in consecutive pairs, and that each UPDATE contains the
+    expected redacted value.

	Redacts and deletes instances of this model where ``field`` equals ``value``.
	Optionally redacts and always deletes instances of this model where ``field`` equals ``value``.

		If ``redact_before_delete_fields`` returns a non-empty dict, matching
		records are bulk-updated with those redacted values before delete.

Conversation

ktyagiapphelix2u commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Ticket & Reference

Related PR

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vgulati-apphelix commented May 6, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktyagiapphelix2u May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ktyagiapphelix2u commented Apr 23, 2026 •

edited

Loading

ktyagiapphelix2u May 28, 2026 •

edited

Loading