-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix: Redact uid field and delete records for retired users from support_historicalusersocialauth #38658
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?
fix: Redact uid field and delete records for retired users from support_historicalusersocialauth #38658
Changes from all commits
b593861
caf150a
bf1db7b
721cd94
6563246
7d01b68
8b9de00
c19fe2f
a411cb5
f813c0a
632871c
c726942
0decc17
a5f35d8
f82889d
e0ddc2f
dbb5c15
73e482b
798d6e5
ff8050d
29d3f71
14c3906
164b22b
0d76944
4142802
9e9d359
319e507
ac78971
d5ab640
96dffa9
193910e
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 |
|---|---|---|
| @@ -1,4 +1,6 @@ | ||
| """ Unit tests for custom UserProfile properties. """ | ||
| """ | ||
| Unit tests for user account utility functions, including social links, completion, and social-auth PII redaction. | ||
| """ | ||
|
|
||
| from contextlib import contextmanager | ||
|
|
||
|
|
@@ -9,16 +11,19 @@ | |
| from django.db.models.signals import pre_delete | ||
| from django.test import TestCase | ||
| from django.test.utils import CaptureQueriesContext, override_settings | ||
| from django.utils import timezone | ||
| 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.signals import redact_social_auth_pii_before_deletion | ||
| from openedx.core.djangoapps.user_api.accounts.utils import ( | ||
| REDACTED_SOCIAL_AUTH_UID_PREFIX, | ||
| redact_and_delete_historical_social_auth, | ||
| redact_and_delete_social_auth, | ||
| retrieve_last_sitewide_block_completed, | ||
| ) | ||
| from openedx.core.djangolib.testing.utils import skip_unless_lms | ||
| from openedx.core.djangolib.testing.utils import assert_redact_before_delete, skip_unless_lms | ||
| from xmodule.modulestore.tests.django_utils import ( | ||
| SharedModuleStoreTestCase, # pylint: disable=wrong-import-order | ||
| ) | ||
|
|
@@ -30,26 +35,6 @@ | |
| from ..utils import format_social_link, validate_social_link | ||
|
|
||
|
|
||
| def assert_update_before_delete(sql_list, num_redact_delete_pairs=1, table='social_auth_usersocialauth'): | ||
| """ | ||
| Assert that UPDATE and DELETE queries for ``table`` occur in consecutive pairs. | ||
| """ | ||
| table_key = table.upper() | ||
| expected_sql_list = [ | ||
| sql for sql in sql_list | ||
| if table_key in sql.upper() and ('UPDATE' in sql.upper() or 'DELETE' in sql.upper()) | ||
| ] | ||
| assert len(expected_sql_list) == num_redact_delete_pairs * 2, ( | ||
| f'Expected {num_redact_delete_pairs * 2} UPDATE/DELETE queries on {table}, ' | ||
| f'got {len(expected_sql_list)}' | ||
| ) | ||
|
|
||
| for index in range(0, len(expected_sql_list), 2): | ||
| update_sql = expected_sql_list[index] | ||
| delete_sql = expected_sql_list[index + 1] | ||
| assert 'UPDATE' in update_sql.upper(), f'Expected UPDATE at position {index} for {table}' | ||
| assert 'DELETE' in delete_sql.upper(), f'Expected DELETE at position {index + 1} for {table}' | ||
|
|
||
| # Use a context manager to guarantee signal reconnection between tests. | ||
| @contextmanager | ||
| def disconnected_social_auth_redaction_signal(): | ||
|
|
@@ -203,6 +188,21 @@ def create_social_auth(self, provider='google-oauth2', uid='user@example.com', e | |
| extra_data=extra_data, | ||
| ) | ||
|
|
||
| def _assert_redact_and_delete_social_auth(self, social_auth_ids): | ||
| """ | ||
| Test redact_and_delete_social_auth and assert that all given records were | ||
| redacted before deletion. | ||
| """ | ||
| with disconnected_social_auth_redaction_signal(), CaptureQueriesContext(connection) as ctx: | ||
| redact_and_delete_social_auth(self.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. This isn't part of the assertion, but the heart of what is getting tested. It don't make sense for this to be in the helper. I think a better solution would be a single test with no helper. You could use ddt to set up the list of users (1 and 2) for the two tests. |
||
|
|
||
| assert_redact_before_delete( | ||
| [query['sql'] for query in ctx], | ||
| table='social_auth_usersocialauth', | ||
|
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. We've been getting the table names from the model. |
||
| expected_redacted_value_list=[REDACTED_SOCIAL_AUTH_UID_PREFIX], | ||
| ) | ||
| assert not UserSocialAuth.objects.filter(id__in=social_auth_ids).exists() | ||
|
|
||
| def test_redact_and_delete_redacts_single_sso_record(self): | ||
| """ | ||
| Test that redact_and_delete_social_auth redacts and deletes a single SSO record. | ||
|
|
@@ -212,13 +212,7 @@ def test_redact_and_delete_redacts_single_sso_record(self): | |
| uid='google@example.com', | ||
| extra_data={'email': 'google@example.com', 'name': 'Google User'}, | ||
| ) | ||
| social_auth_id = social_auth.pk | ||
|
|
||
| with disconnected_social_auth_redaction_signal(), CaptureQueriesContext(connection) as ctx: | ||
| redact_and_delete_social_auth(self.user.id) | ||
|
|
||
| assert_update_before_delete([query['sql'] for query in ctx]) | ||
| assert not UserSocialAuth.objects.filter(id=social_auth_id).exists() | ||
| self._assert_redact_and_delete_social_auth([social_auth.pk]) | ||
|
|
||
| def test_redact_and_delete_redacts_multiple_sso_records(self): | ||
This comment was marked as resolved.
Sorry, something went wrong.
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. Implemented. |
||
| """ | ||
|
|
@@ -236,9 +230,67 @@ def test_redact_and_delete_redacts_multiple_sso_records(self): | |
| extra_data={'email': 'saml@example.com', 'name': 'SAML User', 'uid': 'saml-uid'}, | ||
| ).pk, | ||
| ] | ||
| self._assert_redact_and_delete_social_auth(social_auth_ids) | ||
|
|
||
| with disconnected_social_auth_redaction_signal(), CaptureQueriesContext(connection) as ctx: | ||
| redact_and_delete_social_auth(self.user.id) | ||
|
|
||
| assert_update_before_delete([query['sql'] for query in ctx]) | ||
| assert not UserSocialAuth.objects.filter(id__in=social_auth_ids).exists() | ||
| @skip_unless_lms | ||
| class RedactAndDeleteHistoricalSocialAuthTest(TestCase): | ||
| """ | ||
| Tests for the redact_and_delete_historical_social_auth utility function. | ||
| """ | ||
|
|
||
| def setUp(self): | ||
| super().setUp() | ||
| self.user = UserFactory.create(username='testuser', email='testuser@example.com') | ||
| self.historical_social_auth_model = UserSocialAuth.history.model | ||
|
|
||
| def _create_historical_record(self, provider='google-oauth2', uid='user@example.com', extra_data=None, source_id=1): | ||
| """ | ||
| Create a HistoricalUserSocialAuth record directly for test setup. | ||
| """ | ||
| if extra_data is None: | ||
| extra_data = {'email': uid, 'name': 'Test User'} | ||
| return self.historical_social_auth_model.objects.create( | ||
| user=self.user, | ||
| id=source_id, | ||
| provider=provider, | ||
| uid=uid, | ||
| extra_data=extra_data, | ||
| created=timezone.now(), | ||
| modified=timezone.now(), | ||
| history_date=timezone.now(), | ||
| history_type='+', | ||
| ) | ||
|
|
||
| def test_historical_social_auth_redact_before_delete(self): | ||
| """ | ||
| Ensure HistoricalUserSocialAuth records are properly redacted and deleted for retirement. | ||
|
|
||
| The fields uid (email format) and extra_data must be redacted before delete. | ||
| """ | ||
| self._create_historical_record(provider='google-oauth2', uid='google@example.com', source_id=1) | ||
| self._create_historical_record(provider='tpa-saml', uid='saml@example.com', source_id=2) | ||
|
|
||
| other_user = UserFactory.create(username='otheruser', email='other@example.com') | ||
| other_record = self.historical_social_auth_model.objects.create( | ||
|
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. You have |
||
| user=other_user, | ||
| id=3, | ||
| provider='google-oauth2', | ||
| uid='other@example.com', | ||
| extra_data={}, | ||
| created=timezone.now(), | ||
| modified=timezone.now(), | ||
| history_date=timezone.now(), | ||
| history_type='+', | ||
| ) | ||
|
|
||
| with CaptureQueriesContext(connection) as ctx: | ||
| redact_and_delete_historical_social_auth(self.user.id) | ||
|
|
||
| assert_redact_before_delete( | ||
| [query['sql'] for query in ctx], | ||
| table=self.historical_social_auth_model._meta.db_table, | ||
| expected_redacted_value_list=[REDACTED_SOCIAL_AUTH_UID_PREFIX], | ||
| ) | ||
| assert not self.historical_social_auth_model.objects.filter(user=self.user).exists() | ||
| assert self.historical_social_auth_model.objects.filter(history_id=other_record.history_id).exists() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,10 +22,11 @@ | |
| from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import ( | ||
| setup_retirement_states, # noqa: F401 | ||
| ) | ||
| from openedx.core.djangoapps.user_api.accounts.tests.test_utils import ( | ||
| assert_update_before_delete, | ||
| from openedx.core.djangoapps.user_api.accounts.utils import REDACTED_SOCIAL_AUTH_UID_PREFIX | ||
| from openedx.core.djangolib.testing.utils import ( # pylint: disable=wrong-import-order | ||
| assert_redact_before_delete, | ||
| skip_unless_lms, | ||
| ) | ||
| from openedx.core.djangolib.testing.utils import skip_unless_lms # pylint: disable=wrong-import-order | ||
|
|
||
| from ...models import UserRetirementStatus | ||
|
|
||
|
|
@@ -164,7 +165,11 @@ def test_retire_user_redacts_sso_pii_before_deletion(setup_retirement_states, so | |
| with disconnected_social_auth_redaction_signal(), CaptureQueriesContext(connection) as ctx: | ||
| call_command('retire_user', username=user.username, user_email=user.email) | ||
|
|
||
| assert_update_before_delete([query['sql'] for query in ctx]) | ||
| assert_redact_before_delete( | ||
| [query['sql'] for query in ctx], | ||
| table='social_auth_usersocialauth', | ||
|
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. Again, we've been getting the table names from the model. |
||
| expected_redacted_value_list=[REDACTED_SOCIAL_AUTH_UID_PREFIX], | ||
| ) | ||
| for auth_id in auth_ids: | ||
| assert not UserSocialAuth.objects.filter(id=auth_id).exists() | ||
|
|
||
|
|
||
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.
Nit: I made minor changes. It'd either leave out the "Includes..." line, or add the "etc." (like I did), to not make it sound comprehensive. It would be easy to add tests without updating this comment, and comments can easily get out of sync.