fix: retirement PII leaks by redacting pending secondary email/name data#227
fix: retirement PII leaks by redacting pending secondary email/name data#227ktyagiapphelix2u wants to merge 4 commits intorelease-ulmofrom
Conversation
There was a problem hiding this comment.
Pull request overview
Closes retirement-related PII leak gaps by ensuring pending secondary email change records and account recovery secondary emails are redacted before deletion across retirement entry points, preventing downstream soft-delete mirrors from retaining plaintext values.
Changes:
- Added redact-then-delete retirement helpers for pending secondary email changes and improved redaction for account recovery secondary emails.
- Updated retirement flows (API, management command, deactivation utils) to retire pending secondary email/account recovery consistently; hardened pending email/name retirement redaction.
- Added/updated tests to validate secondary email cleanup across flows.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| openedx/core/djangoapps/user_api/management/tests/test_retire_user.py | Adds management-command test coverage for pending secondary email cleanup. |
| openedx/core/djangoapps/user_api/management/commands/retire_user.py | Ensures management command retires pending secondary email records. |
| openedx/core/djangoapps/user_api/accounts/views.py | Hardens retirement endpoints: redact pending name/email before delete; retires secondary/account recovery; sets user.email to retired value. |
| openedx/core/djangoapps/user_api/accounts/utils.py | Ensures deactivation flow retires pending secondary email records. |
| openedx/core/djangoapps/user_api/accounts/tests/test_retirement_views.py | Adds tests to validate pending secondary + account recovery cleanup during retirement. |
| common/djangoapps/student/views/management.py | Redacts pending secondary email before deletion during activation flow. |
| common/djangoapps/student/tests/test_models.py | Adds model-level tests for retiring pending secondary email changes. |
| common/djangoapps/student/models/user.py | Introduces PendingSecondaryEmailChange.retire_pending_secondary_email and updates AccountRecovery retirement to redact before delete. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| RevisionPluginRevision.retire_user(retirement.user) | ||
| ArticleRevision.retire_user(retirement.user) | ||
| # Redact PendingNameChange before deletion to prevent plaintext sync to Snowflake | ||
| PendingNameChange.objects.filter(user=retirement.user).update(new_name="", rationale="") |
There was a problem hiding this comment.
@ktyagiapphelix2u, Do we not want to redact the field instead of making this empty string?
There was a problem hiding this comment.
Yes I have updated the code with redaction
| activation_key = models.CharField(('activation key'), max_length=32, unique=True, db_index=True) | ||
|
|
||
| @classmethod | ||
| def retire_pending_secondary_email(cls, user_id): |
There was a problem hiding this comment.
@ktyagiapphelix2u , you may want to rename this function to redact_pending_secondary_email instead?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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, retired in `DeactivateLogoutView` |
There was a problem hiding this comment.
I would reserve the word "retired" to mean "redacted and can't re-use". Here, I would instead use the word "redacted".
| @classmethod | ||
| def redact_pending_secondary_email(cls, user_id): | ||
| """ | ||
| Retire a pending secondary email change row for a user. |
There was a problem hiding this comment.
Please search all your changes for "retire" and see if there is anything else that should be changed like this:
| Retire a pending secondary email change row for a user. | |
| Redact a pending secondary email change row for a user. |
| pass | ||
| return True | ||
|
|
||
| account_recovery.secondary_email = get_retired_email_by_email(account_recovery.secondary_email) |
There was a problem hiding this comment.
What does get_retired_email_by_email do, and to we want that? We only want to redact, and not retire.
Would be good enough to use a hard-coded value like 'redacted@redacted.com`? Reminder that this record is about to be deleted, so this value should mostly never be seen. We just want it to be a valid email format in case the DB would reject an empty string, etc.
|
@ktyagiapphelix2u: Can you confirm that this PR is only fixing issues of redacting before deletion, or are there any places where the email lived on forever in the LMS database? Thanks. |
This PR only fixes redaction-before-deletion for secondary email records. It does not change retention behavior in LMS; those records are still deleted as before. And Places I check for the certificates table and partner report tables that would be getting in deleted after their own cool off period of 60 days I think we should not handle them here we can do them seprately but they get deleted from the lms database so we are good to go there. |
Summary
This PR closes retirement-related PII leak gaps for secondary email and related pending records in LMS.
It ensures all retirement entry points consistently redact sensitive fields before deletion, so downstream soft-delete mirrors do not retain original plaintext values.
What changed
Added retirement cleanup for pending secondary email records in all retirement flows.
Added retirement cleanup for account recovery secondary email in all retirement flows.
Updated admin retirement flow to also set user email to retired hashed value.
Hardened pending email retirement handling to redact pending email before delete.
Hardened pending name retirement handling to clear pending name/rationale before delete.
Kept existing retirement behavior for profile/certificate/social data and validated coverage.
Added/updated tests for:
Secondary email cleanup on deactivation flow
Secondary email cleanup on management command flow
Secondary/account recovery cleanup on admin retirement flow
Why
Before this change, some retirement paths could leave or soft-delete plaintext PII (especially pending secondary email, pending email change, and pending name change records).
This PR enforces redact-then-delete behavior consistently across API and command paths.
Ticket & Reference
https://2u-internal.atlassian.net/browse/BOMS-499
https://2u-internal.atlassian.net/wiki/spaces/AT/pages/3503652877/Secondary+Email+BOMS+-+499