-
Notifications
You must be signed in to change notification settings - Fork 11
fix: retirement PII leaks by redacting pending secondary email/name data #227
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 1 commit
071e3fb
0d70442
a0deee3
59c9308
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -933,14 +933,34 @@ class PendingSecondaryEmailChange(DeletableByUserValue, models.Model): | |||||
| """ | ||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would reserve the word "retired" to mean "redacted and can't re-use". Here, I would instead use the word "redacted". |
||||||
| .. pii_types: email_address | ||||||
| .. pii_retirement: retained | ||||||
| .. pii_retirement: local_api | ||||||
| """ | ||||||
| user = models.OneToOneField(User, unique=True, db_index=True, on_delete=models.CASCADE) | ||||||
| new_secondary_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 retire_pending_secondary_email(cls, user_id): | ||||||
|
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 , you may want to rename this function to redact_pending_secondary_email instead? |
||||||
| """ | ||||||
| Retire a pending secondary email change row for a user. | ||||||
|
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. Please search all your changes for "retire" and see if there is anything else that should be changed like this:
Suggested change
|
||||||
|
|
||||||
| Redacts the email before deletion so any downstream soft-delete mirror does | ||||||
| not retain the original secondary email address in the final row image. | ||||||
| """ | ||||||
| try: | ||||||
| pending_secondary_email = cls.objects.get(user_id=user_id) | ||||||
| except cls.DoesNotExist: | ||||||
| return True | ||||||
|
|
||||||
| pending_secondary_email.new_secondary_email = get_retired_email_by_email( | ||||||
| pending_secondary_email.new_secondary_email | ||||||
| ) | ||||||
| pending_secondary_email.save(update_fields=['new_secondary_email']) | ||||||
| pending_secondary_email.delete() | ||||||
| return True | ||||||
|
|
||||||
|
|
||||||
| class LoginFailures(models.Model): | ||||||
| """ | ||||||
|
|
@@ -1690,16 +1710,21 @@ def retire_recovery_email(cls, user_id): | |||||
| Retire user's recovery/secondary email as part of GDPR Phase I. | ||||||
| Returns 'True' | ||||||
|
|
||||||
| If an AccountRecovery record is found for this user it will be deleted, | ||||||
| if it is not found it is assumed this table has no PII for the given user. | ||||||
| If an AccountRecovery record is found for this user it will be redacted and | ||||||
| deleted. If it is not found it is assumed this table has no PII for the given user. | ||||||
|
|
||||||
| :param user_id: int | ||||||
| :return: bool | ||||||
| """ | ||||||
| try: | ||||||
| cls.objects.get(user_id=user_id).delete() | ||||||
| account_recovery = cls.objects.get(user_id=user_id) | ||||||
| except cls.DoesNotExist: | ||||||
| pass | ||||||
| return True | ||||||
|
|
||||||
| account_recovery.secondary_email = get_retired_email_by_email(account_recovery.secondary_email) | ||||||
|
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. What does Would be good enough to use a hard-coded value like '[email protected]`? 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. |
||||||
| account_recovery.is_active = False | ||||||
| account_recovery.save(update_fields=['secondary_email', 'is_active']) | ||||||
| account_recovery.delete() | ||||||
|
|
||||||
| return True | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |
| ManualEnrollmentAudit, | ||
| PendingEmailChange, | ||
| PendingNameChange, | ||
| PendingSecondaryEmailChange, | ||
| UserAttribute, | ||
| UserCelebration, | ||
| UserProfile | ||
|
|
@@ -745,6 +746,29 @@ def test_retire_recovery_email(self): | |
| assert len(AccountRecovery.objects.filter(user_id=user.id)) == 0 | ||
|
|
||
|
|
||
| class TestPendingSecondaryEmailChange(TestCase): | ||
| """Tests for retiring PendingSecondaryEmailChange records.""" | ||
|
|
||
| def test_retire_pending_secondary_email(self): | ||
| """Assert that pending secondary email records are deleted for retired users.""" | ||
| user = UserFactory() | ||
| PendingSecondaryEmailChange.objects.create( | ||
| user=user, | ||
| new_secondary_email='[email protected]', | ||
| activation_key='a' * 32, | ||
| ) | ||
| assert len(PendingSecondaryEmailChange.objects.filter(user_id=user.id)) == 1 | ||
|
|
||
| PendingSecondaryEmailChange.retire_pending_secondary_email(user_id=user.id) | ||
|
|
||
| assert len(PendingSecondaryEmailChange.objects.filter(user_id=user.id)) == 0 | ||
|
|
||
| def test_retire_pending_secondary_email_when_no_record(self): | ||
| """Assert retirement cleanup returns True when no pending secondary row exists.""" | ||
| user = UserFactory() | ||
| assert PendingSecondaryEmailChange.retire_pending_secondary_email(user_id=user.id) is True | ||
|
|
||
|
|
||
| @ddt.ddt | ||
| class TestUserPostSaveCallback(SharedModuleStoreTestCase): | ||
| """ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ | |
| ManualEnrollmentAudit, | ||
| PendingEmailChange, | ||
| PendingNameChange, | ||
| PendingSecondaryEmailChange, | ||
| Registration, | ||
| SocialLink, | ||
| UserProfile, | ||
|
|
@@ -235,6 +236,24 @@ def test_user_can_deactivate_secondary_email(self): | |
| # Assert that there is no longer a secondary/recovery email for test user | ||
| assert len(AccountRecovery.objects.filter(user_id=self.test_user.id)) == 0 | ||
|
|
||
| def test_user_can_deactivate_pending_secondary_email_change(self): | ||
| """ | ||
| Verify that pending secondary email change records are removed when a user retires. | ||
| """ | ||
| PendingSecondaryEmailChange.objects.create( | ||
| user=self.test_user, | ||
| new_secondary_email='[email protected]', | ||
| activation_key='b' * 32, | ||
| ) | ||
| assert len(PendingSecondaryEmailChange.objects.filter(user_id=self.test_user.id)) == 1 | ||
|
|
||
| self.client.login(username=self.test_user.username, password=self.test_password) | ||
| headers = build_jwt_headers(self.test_user) | ||
| response = self.client.post(self.url, self.build_post(self.test_password), **headers) | ||
| assert response.status_code == status.HTTP_204_NO_CONTENT | ||
|
|
||
| assert len(PendingSecondaryEmailChange.objects.filter(user_id=self.test_user.id)) == 0 | ||
|
|
||
| def test_password_mismatch(self): | ||
| """ | ||
| Verify that the user submitting a mismatched password results in | ||
|
|
@@ -1393,6 +1412,18 @@ def setUp(self): | |
| PendingEmailChangeFactory.create(user=self.test_user) | ||
| UserOrgTagFactory.create(user=self.test_user, key='foo', value='bar') | ||
| UserOrgTagFactory.create(user=self.test_user, key='cat', value='dog') | ||
|
|
||
|
ktyagiapphelix2u marked this conversation as resolved.
Outdated
|
||
| # Secondary email setup | ||
| PendingSecondaryEmailChange.objects.create( | ||
| user=self.test_user, | ||
| new_secondary_email='[email protected]', | ||
| activation_key='test_activation_key_123' | ||
| ) | ||
| AccountRecovery.objects.create( | ||
| user=self.test_user, | ||
| secondary_email='[email protected]', | ||
| is_active=True | ||
| ) | ||
|
|
||
| CourseEnrollmentAllowedFactory.create(email=self.original_email) | ||
|
|
||
|
|
@@ -1499,6 +1530,10 @@ def test_retire_user(self, mock_remove_profile_images, mock_get_profile_image_na | |
|
|
||
| assert not PendingEmailChange.objects.filter(user=self.test_user).exists() | ||
| assert not UserOrgTag.objects.filter(user=self.test_user).exists() | ||
|
|
||
|
ktyagiapphelix2u marked this conversation as resolved.
Outdated
|
||
| # Verify secondary email models were cleaned | ||
| assert not PendingSecondaryEmailChange.objects.filter(user=self.test_user).exists() | ||
| assert not AccountRecovery.objects.filter(user=self.test_user).exists() | ||
|
|
||
| assert not CourseEnrollmentAllowed.objects.filter(email=self.original_email).exists() | ||
| assert not UnregisteredLearnerCohortAssignments.objects.filter(email=self.original_email).exists() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,11 +39,13 @@ | |
| from common.djangoapps.track import segment | ||
| from common.djangoapps.entitlements.models import CourseEntitlement | ||
| from common.djangoapps.student.models import ( # lint-amnesty, pylint: disable=unused-import | ||
| AccountRecovery, | ||
| CourseEnrollmentAllowed, | ||
| LoginFailures, | ||
| ManualEnrollmentAudit, | ||
| PendingEmailChange, | ||
| PendingNameChange, | ||
| PendingSecondaryEmailChange, | ||
| User, | ||
| UserProfile, | ||
| get_potentially_retired_user_by_username, | ||
|
|
@@ -1099,6 +1101,8 @@ def post(self, request): | |
| retirement = UserRetirementStatus.get_retirement_for_retirement_action(username) | ||
| 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="") | ||
|
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, Do we not want to redact the field instead of making this empty string?
Author
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. Yes I have updated the code with redaction |
||
| PendingNameChange.delete_by_user_value(retirement.user, field="user") | ||
| ManualEnrollmentAudit.retire_manual_enrollments(retirement.user, retirement.retired_email) | ||
|
|
||
|
|
@@ -1195,8 +1199,15 @@ def post(self, request): | |
| self.retire_entitlement_support_detail(user) | ||
|
|
||
| # Retire misc. models that may contain PII of this user | ||
| # Redact pending email change before deletion to prevent plaintext sync to Snowflake | ||
| pending_email = PendingEmailChange.objects.filter(user=user).first() | ||
| if pending_email: | ||
| pending_email.new_email = get_retired_email_by_email(pending_email.new_email) | ||
| pending_email.save(update_fields=['new_email']) | ||
| PendingEmailChange.delete_by_user_value(user, field="user") | ||
| UserOrgTag.delete_by_user_value(user, field="user") | ||
| PendingSecondaryEmailChange.retire_pending_secondary_email(user.id) | ||
| AccountRecovery.retire_recovery_email(user.id) | ||
|
|
||
| # Retire any objects linked to the user via their original email | ||
| CourseEnrollmentAllowed.delete_by_user_value(original_email, field="email") | ||
|
|
@@ -1214,6 +1225,7 @@ def post(self, request): | |
| user.last_name = "" | ||
| user.is_active = False | ||
| user.username = retired_username | ||
| user.email = retired_email | ||
| user.save() | ||
|
ktyagiapphelix2u marked this conversation as resolved.
|
||
| except UserRetirementStatus.DoesNotExist: | ||
| return Response(status=status.HTTP_404_NOT_FOUND) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user | ||
| from django.core.management import CommandError, call_command | ||
|
|
||
| from common.djangoapps.student.models import PendingSecondaryEmailChange | ||
| from ...models import UserRetirementStatus | ||
| from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import ( # lint-amnesty, pylint: disable=unused-import, wrong-import-order | ||
| setup_retirement_states | ||
|
|
@@ -105,3 +106,19 @@ def test_retire_with_username_email_userfile(setup_retirement_states): # lint-a | |
| with pytest.raises(CommandError, match=r'You cannot use userfile option with username and user_email'): | ||
| call_command('retire_user', user_file=user_file, username=username, user_email=user_email) | ||
| remove_user_file() | ||
|
|
||
|
|
||
| @skip_unless_lms | ||
| def test_retire_user_cleans_pending_secondary_email(setup_retirement_states): # lint-amnesty, pylint: disable=redefined-outer-name, unused-argument | ||
| user = UserFactory.create(username='user-cleanup', email='[email protected]') | ||
| PendingSecondaryEmailChange.objects.create( | ||
| user=user, | ||
| new_secondary_email='[email protected]', | ||
| activation_key='c' * 32, | ||
| ) | ||
|
|
||
| assert PendingSecondaryEmailChange.objects.filter(user=user).exists() | ||
|
|
||
| call_command('retire_user', username=user.username, user_email=user.email) | ||
|
|
||
| assert not PendingSecondaryEmailChange.objects.filter(user=user).exists() | ||
Uh oh!
There was an error while loading. Please reload this page.