-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix: Redact SSO PII before deletion #38425
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: master
Are you sure you want to change the base?
Changes from all commits
9a178e3
8d57698
2688ac8
ff4b57e
417aa3d
542b5be
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 |
|---|---|---|
|
|
@@ -6,10 +6,14 @@ | |
| from completion.test_utils import CompletionWaffleTestMixin | ||
| from django.test import TestCase | ||
| from django.test.utils import override_settings | ||
| from social_django.models import UserSocialAuth | ||
|
|
||
| from common.djangoapps.student.models import CourseEnrollment | ||
| from common.djangoapps.student.tests.factories import UserFactory | ||
| from openedx.core.djangoapps.user_api.accounts.utils import retrieve_last_sitewide_block_completed | ||
| from openedx.core.djangoapps.user_api.accounts.utils import ( | ||
| redact_user_social_auth_pii, | ||
| retrieve_last_sitewide_block_completed, | ||
| ) | ||
| from openedx.core.djangolib.testing.utils import skip_unless_lms | ||
| from xmodule.modulestore.tests.django_utils import ( | ||
| SharedModuleStoreTestCase, # lint-amnesty, pylint: disable=wrong-import-order | ||
|
|
@@ -133,3 +137,79 @@ def test_retrieve_last_sitewide_block_completed(self): | |
| ) | ||
|
|
||
| assert empty_block_url is None | ||
|
|
||
|
|
||
| @skip_unless_lms | ||
| class RedactUserSocialAuthPIITest(TestCase): | ||
| """ | ||
| Tests for SSO PII redaction before deletion. | ||
| """ | ||
|
|
||
| def setUp(self): | ||
| super().setUp() | ||
| self.user = UserFactory.create(username='testuser', email='testuser@example.com') | ||
|
|
||
| def create_social_auth(self, provider='google-oauth2', uid='user@example.com', extra_data=None): | ||
| """ | ||
| Helper method to create UserSocialAuth instances for testing. | ||
| """ | ||
| if extra_data is None: | ||
| extra_data = { | ||
| 'email': 'user@example.com', | ||
| 'name': 'Test User', | ||
| 'id': '123456789', | ||
| } | ||
| return UserSocialAuth.objects.create( | ||
| user=self.user, | ||
| provider=provider, | ||
| uid=uid, | ||
| extra_data=extra_data, | ||
| ) | ||
|
|
||
| def test_redact_user_social_auth_pii(self): | ||
| """ | ||
| Test that redact_user_social_auth_pii correctly redacts uid and extra_data fields. | ||
| """ | ||
| social_auth = self.create_social_auth() | ||
|
|
||
| redact_user_social_auth_pii(social_auth) | ||
| social_auth.refresh_from_db() | ||
|
|
||
| assert social_auth.uid == f'redacted_{social_auth.pk}@retired.invalid' | ||
| assert social_auth.extra_data == {} | ||
|
|
||
| def test_redact_user_social_auth_pii_idempotent(self): | ||
| """ | ||
| Test that calling redact_user_social_auth_pii multiple times is idempotent. | ||
| """ | ||
| social_auth = self.create_social_auth() | ||
|
|
||
| redact_user_social_auth_pii(social_auth) | ||
| # Duplicate call to redact user method to validate idempotency | ||
| redact_user_social_auth_pii(social_auth) | ||
|
Contributor
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 may be adding a comment on line#189 will help. "duplicate call to redact user method to validate idempotency"
Contributor
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. Added |
||
| social_auth.refresh_from_db() | ||
|
|
||
| assert social_auth.uid == f'redacted_{social_auth.pk}@retired.invalid' | ||
| assert social_auth.extra_data == {} | ||
|
|
||
| def test_redact_multiple_sso_providers(self): | ||
| """ | ||
| Test that redaction works correctly for multiple SSO providers. | ||
| """ | ||
| auths = [ | ||
| self.create_social_auth( | ||
| provider='google-oauth2', | ||
| uid='google@example.com', | ||
| extra_data={'email': 'google@example.com', 'name': 'Google User'} | ||
| ), | ||
| self.create_social_auth( | ||
| provider='tpa-saml', | ||
| uid='saml@example.com', | ||
| extra_data={'email': 'saml@example.com', 'name': 'SAML User', 'uid': 'saml-uid'} | ||
| ), | ||
| ] | ||
| for auth in auths: | ||
| redact_user_social_auth_pii(auth) | ||
| auth.refresh_from_db() | ||
| assert auth.uid == f'redacted_{auth.pk}@retired.invalid' | ||
| assert auth.extra_data == {} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| from common.djangoapps.student.models import AccountRecovery, Registration, get_retired_email_by_email | ||
| from openedx.core.djangolib.oauth2_retirement_utils import retire_dot_oauth2_models | ||
|
|
||
| from ...accounts.utils import redact_user_social_auth_pii | ||
| from ...models import BulkUserRetirementConfig, UserRetirementStatus | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
@@ -144,8 +145,10 @@ def handle(self, *args, **options): | |
| for user in users: | ||
| # Add user to retirement queue. | ||
| UserRetirementStatus.create_retirement(user) | ||
| # Unlink LMS social auth accounts | ||
| UserSocialAuth.objects.filter(user_id=user.id).delete() | ||
| # Redact and unlink LMS social auth accounts | ||
| for social_auth in UserSocialAuth.objects.filter(user_id=user.id): | ||
|
Contributor
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.
|
||
| redact_user_social_auth_pii(social_auth) | ||
| social_auth.delete() | ||
| # Change LMS password & email | ||
| user.email = get_retired_email_by_email(user.email) | ||
| user.set_unusable_password() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,10 +5,12 @@ | |
|
|
||
| import csv | ||
| import os | ||
| from unittest import mock | ||
|
|
||
| import pytest | ||
| from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user | ||
| from django.core.management import CommandError, call_command | ||
| from social_django.models import UserSocialAuth | ||
|
|
||
| from common.djangoapps.student.tests.factories import UserFactory # lint-amnesty, pylint: disable=wrong-import-order | ||
| from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import ( # lint-amnesty, pylint: disable=unused-import, wrong-import-order | ||
|
|
@@ -107,3 +109,81 @@ 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_redacts_sso_pii_before_deletion(setup_retirement_states): # lint-amnesty, pylint: disable=redefined-outer-name, unused-argument # noqa: F811 | ||
|
Contributor
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. This test only checks the record was deleted — it doesn't verify redaction happened before deletion. Use a
Contributor
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. Updated the test Thanks |
||
| """ | ||
| Test that SSO PII is redacted before UserSocialAuth records are deleted during retirement. | ||
|
|
||
| This test verifies the order of operations by capturing the record's state | ||
| at the moment of deletion to ensure it was already redacted. | ||
| """ | ||
| user = UserFactory.create(username='sso-user', email='sso-user@example.com') | ||
| social_auth = UserSocialAuth.objects.create( | ||
| user=user, | ||
| provider='google-oauth2', | ||
| uid='sso-user@example.com', | ||
| extra_data={ | ||
| 'email': 'sso-user@example.com', | ||
| 'name': 'SSO Test User', | ||
| 'id': '123456789', | ||
| } | ||
| ) | ||
| social_auth_id = social_auth.id | ||
|
|
||
| # Capture the state at the moment of deletion to verify redaction happened first | ||
| captured_state = {} | ||
| original_delete = UserSocialAuth.delete | ||
|
|
||
| def capture_state_and_delete(self): | ||
| """Wrapper to capture state before deletion.""" | ||
| # Refresh from database to get the actual current state | ||
| self.refresh_from_db() | ||
| captured_state['uid'] = self.uid | ||
| captured_state['extra_data'] = dict(self.extra_data) if self.extra_data else {} | ||
| # Call original delete | ||
| return original_delete(self) | ||
|
|
||
| with mock.patch.object(UserSocialAuth, 'delete', capture_state_and_delete): | ||
| call_command('retire_user', username=user.username, user_email=user.email) | ||
|
|
||
| # Verify that at the moment of deletion, the record was already redacted | ||
| assert captured_state['uid'] == f'redacted_{social_auth_id}@retired.invalid', \ | ||
| "UID should be redacted before deletion" | ||
| assert captured_state['extra_data'] == {}, \ | ||
| "extra_data should be empty before deletion" | ||
|
|
||
| # Verify deletion completed | ||
| assert not UserSocialAuth.objects.filter(id=social_auth_id).exists() | ||
|
|
||
| retired_user_status = UserRetirementStatus.objects.filter(original_username=user.username).first() | ||
| assert retired_user_status is not None | ||
| assert retired_user_status.original_email == 'sso-user@example.com' | ||
|
|
||
|
|
||
| @skip_unless_lms | ||
| def test_retire_user_calls_redaction_for_each_social_auth(setup_retirement_states): # lint-amnesty, pylint: disable=redefined-outer-name, unused-argument # noqa: F811 | ||
| """ | ||
| Test that redact_user_social_auth_pii is called for each UserSocialAuth record during retirement. | ||
| """ | ||
| user = UserFactory.create(username='multi-sso-user', email='multi-sso@example.com') | ||
| UserSocialAuth.objects.create( | ||
| user=user, | ||
| provider='google-oauth2', | ||
| uid='google-multi@example.com', | ||
| extra_data={'email': 'google-multi@example.com', 'name': 'Google User'} | ||
| ) | ||
| UserSocialAuth.objects.create( | ||
| user=user, | ||
| provider='tpa-saml', | ||
| uid='saml-multi@example.com', | ||
| extra_data={'email': 'saml-multi@example.com', 'name': 'SAML User', 'uid': 'saml-123'} | ||
| ) | ||
|
|
||
| with mock.patch( | ||
| 'openedx.core.djangoapps.user_api.management.commands.retire_user.redact_user_social_auth_pii' | ||
| ) as mock_redact: | ||
| call_command('retire_user', username=user.username, user_email=user.email) | ||
|
|
||
| assert mock_redact.call_count == 2 | ||
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.
Swallowing the exception here means if redaction fails, deletion proceeds with PII intact — exactly what this PR is meant to prevent. Either re-raise after logging, or explicitly document "best-effort" as the intended part.
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.
Done! I've updated the signal handler to re-raise the exception after logging.
Before:
Redaction fails → Exception logged → Deletion proceeds → PII leaked
After:
Redaction fails → Exception logged → Exception re-raised → Deletion blocked → PII protected