-
Notifications
You must be signed in to change notification settings - Fork 11
fix: redact pending primary email before retirement delete #229
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.
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
base: release-ulmo
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1466,7 +1466,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) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
@@ -1497,6 +1503,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() | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
@@ -1509,6 +1516,24 @@ 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_before_retirement = PendingEmailChange.objects.get(user=self.test_user).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 | ||||||||||||||||||||||||||||||||
|
Comment on lines
+1521
to
+1526
|
||||||||||||||||||||||||||||||||
| pending_email_before_retirement = PendingEmailChange.objects.get(user=self.test_user).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_email_record = PendingEmailChange.objects.get(user=self.test_user) | |
| pending_email_before_retirement = pending_email_record.new_email | |
| activation_key_before_retirement = pending_email_record.activation_key | |
| 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 | |
| assert pending_record.activation_key != activation_key_before_retirement |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1193,7 +1193,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 primary email fields before delete because | ||
| # downstream replication can preserve soft-deleted snapshots. | ||
| PendingEmailChange.redact_pending_email_by_user_value(user, field="user") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ktyagiapphelix2u , Although this may not matter much but reaction value is user here, can you please confirm? |
||
| PendingEmailChange.delete_by_user_value(user, field="user") | ||
| UserOrgTag.delete_by_user_value(user, field="user") | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redact_pending_email_by_user_valueonly redactsnew_email, but leavesactivation_keyunchanged. The PR description/incident explains Snowflake soft-deleted snapshots can retain both pending email and activation key values, so keepingactivation_keyintact still leaks a sensitive token. Suggest redactingactivation_keyas well (e.g., replace with a new random/unique value to satisfy theunique=Trueconstraint) and update the method/docstring accordingly.