fix: Redact SSO PII before deletion

Author: ktyagiapphelix2u

openedx/core/djangoapps/user_api/accounts/tests/test_signals.py

@@ -42,24 +42,24 @@ def test_get_redacted_social_auth_uid_format(self):
         assert get_redacted_social_auth_uid(42) == 'redacted-before-delete-42@safe.com'
         assert get_redacted_social_auth_uid(1) == 'redacted-before-delete-1@safe.com'
 
-    def test_signal_warns_and_redacts_when_not_already_redacted(self):
+    @patch('openedx.core.djangoapps.user_api.accounts.signals.redact_and_delete_social_auth')
+    def test_signal_warns_and_redacts_when_not_already_redacted(self, mock_redact):
         """
         When a UserSocialAuth is deleted without prior redaction, the signal handler
         should log a warning and call redact_and_delete_social_auth with skip_delete=True.
         """
         social_auth = self._create_social_auth()
 
-        with patch(
-            'openedx.core.djangoapps.user_api.accounts.signals.redact_and_delete_social_auth'
-        ) as mock_redact, self.assertLogs(
+        with self.assertLogs(
             'openedx.core.djangoapps.user_api.accounts.signals', level=logging.WARNING
         ) as log_ctx:
             social_auth.delete()
 
         mock_redact.assert_called_once_with(self.user.id, skip_delete=True)
         assert any('was deleted without first being redacted' in msg for msg in log_ctx.output)
 
-    def test_signal_skips_warning_and_redaction_when_already_redacted(self):
+    @patch('openedx.core.djangoapps.user_api.accounts.signals.redact_and_delete_social_auth')
+    def test_signal_skips_warning_and_redaction_when_already_redacted(self, mock_redact):
         """
         When a UserSocialAuth is already redacted before deletion, the signal handler
         should not log a warning and should not call redact_and_delete_social_auth.
@@ -70,10 +70,7 @@ def test_signal_skips_warning_and_redaction_when_already_redacted(self):
         social_auth.save(update_fields=['uid', 'extra_data'])
         social_auth_id = social_auth.id
 
-        with patch(
-            'openedx.core.djangoapps.user_api.accounts.signals.redact_and_delete_social_auth'
-        ) as mock_redact:
-            social_auth.delete()
+        social_auth.delete()
 
         mock_redact.assert_not_called()
         assert not UserSocialAuth.objects.filter(id=social_auth_id).exists()

openedx/core/djangoapps/user_api/accounts/tests/test_utils.py

@@ -1,5 +1,7 @@
 """ Unit tests for custom UserProfile properties. """
 
+from contextlib import contextmanager
+
 import ddt
 from completion import models
 from completion.test_utils import CompletionWaffleTestMixin
@@ -28,6 +30,39 @@
 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():
+    """
+    Temporarily disconnect the fallback signal so tests exercise the helper path.
+    """
+    pre_delete.disconnect(redact_social_auth_pii_before_deletion, sender=UserSocialAuth)
+    try:
+        yield
+    finally:
+        pre_delete.connect(redact_social_auth_pii_before_deletion, sender=UserSocialAuth)
+
+
 @ddt.ddt
 class UserAccountSettingsTest(TestCase):
     """Unit tests for setting Social Media Links."""
@@ -146,45 +181,12 @@ def test_retrieve_last_sitewide_block_completed(self):
 class RedactAndDeleteSocialAuthTest(TestCase):
     """
     Tests for the redact_and_delete_social_auth utility function.
-
-    The safety-net pre_delete signal handler is disconnected for all tests in this class
-    to verify that redact_and_delete_social_auth itself redacts before deleting,
-    not the fallback signal.
     """
 
-    @classmethod
-    def setUpClass(cls):
-        super().setUpClass()
-        # Disconnect the safety-net signal so tests verify redact_and_delete_social_auth
-        # itself issues the UPDATE before DELETE, not the signal.
-        pre_delete.disconnect(redact_social_auth_pii_before_deletion, sender=UserSocialAuth)
-
-    @classmethod
-    def tearDownClass(cls):
-        super().tearDownClass()
-        # Reconnect the signal after the test class completes.
-        pre_delete.connect(redact_social_auth_pii_before_deletion, sender=UserSocialAuth)
-
     def setUp(self):
         super().setUp()
         self.user = UserFactory.create(username='testuser', email='testuser@example.com')
 
-    def _assert_update_before_delete(self, sql_list, table='social_auth_usersocialauth'):
-        """
-        Assert that an UPDATE on ``table`` runs before the DELETE on ``table``.
-
-        This verifies that redaction happens in the helper itself, not via the
-        fallback pre_delete signal.
-        """
-        table_key = table.upper()
-        update_indices = [i for i, sql in enumerate(sql_list) if 'UPDATE' in sql.upper() and table_key in sql.upper()]
-        delete_indices = [i for i, sql in enumerate(sql_list) if 'DELETE' in sql.upper() and table_key in sql.upper()]
-        assert update_indices, f'Expected at least one UPDATE on {table}'
-        assert delete_indices, f'Expected at least one DELETE on {table}'
-        assert any(update_idx < delete_idx for update_idx in update_indices for delete_idx in delete_indices), (
-            'Expected at least one UPDATE to precede at least one DELETE'
-        )
-
     def create_social_auth(self, provider='google-oauth2', uid='user@example.com', extra_data=None):
         """
         Helper method to create UserSocialAuth instances for testing.
@@ -201,29 +203,42 @@ def create_social_auth(self, provider='google-oauth2', uid='user@example.com', e
             extra_data=extra_data,
         )
 
-    @ddt.data(
-        {
-            'provider': 'google-oauth2',
-            'uid': 'google@example.com',
-            'extra_data': {'email': 'google@example.com', 'name': 'Google User'}
-        },
-        {
-            'provider': 'tpa-saml',
-            'uid': 'saml@example.com',
-            'extra_data': {'email': 'saml@example.com', 'name': 'SAML User', 'uid': 'saml-uid'}
-        }
-    )
-    @ddt.unpack
-    def test_redact_and_delete_redacts_multiple_sso_providers(self, provider, uid, extra_data):
+    def test_redact_and_delete_redacts_single_sso_record(self):
         """
-        Test that redact_and_delete_social_auth redacts and deletes records for
-        multiple SSO providers in a single call.
+        Test that redact_and_delete_social_auth redacts and deletes a single SSO record.
         """
-        social_auth = self.create_social_auth(provider=provider, uid=uid, extra_data=extra_data)
+        social_auth = self.create_social_auth(
+            provider='google-oauth2',
+            uid='google@example.com',
+            extra_data={'email': 'google@example.com', 'name': 'Google User'},
+        )
         social_auth_id = social_auth.pk
 
-        with CaptureQueriesContext(connection) as ctx:
+        with disconnected_social_auth_redaction_signal(), CaptureQueriesContext(connection) as ctx:
             redact_and_delete_social_auth(self.user.id)
 
-        self._assert_update_before_delete([query['sql'] for query in ctx])
+        assert_update_before_delete([query['sql'] for query in ctx])
         assert not UserSocialAuth.objects.filter(id=social_auth_id).exists()
+
+    def test_redact_and_delete_redacts_multiple_sso_records(self):
+        """
+        Test that redact_and_delete_social_auth redacts and deletes all SSO records for a user.
+        """
+        social_auth_ids = [
+            self.create_social_auth(
+                provider='google-oauth2',
+                uid='google@example.com',
+                extra_data={'email': 'google@example.com', 'name': 'Google User'},
+            ).pk,
+            self.create_social_auth(
+                provider='tpa-saml',
+                uid='saml@example.com',
+                extra_data={'email': 'saml@example.com', 'name': 'SAML User', 'uid': 'saml-uid'},
+            ).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__in=social_auth_ids).exists()

openedx/core/djangoapps/user_api/management/tests/test_retire_user.py

@@ -16,20 +16,23 @@
 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.signals import (  # lint-amnesty, pylint: disable=wrong-import-order
+from openedx.core.djangoapps.user_api.accounts.signals import (
     redact_social_auth_pii_before_deletion,
 )
-from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import (  # lint-amnesty, pylint: disable=unused-import, wrong-import-order
+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.djangolib.testing.utils import skip_unless_lms  # pylint: disable=wrong-import-order
 
 from ...models import UserRetirementStatus
 
 pytestmark = pytest.mark.django_db
 user_file = 'userfile.csv'
 
-
+# Use a context manager to guarantee signal reconnection between tests.
 @contextmanager
 def disconnected_social_auth_redaction_signal():
     """
@@ -42,20 +45,6 @@ def disconnected_social_auth_redaction_signal():
         pre_delete.connect(redact_social_auth_pii_before_deletion, sender=UserSocialAuth)
 
 
-def assert_update_before_delete(sql_list, table='social_auth_usersocialauth'):
-    """
-    Assert that at least one social auth UPDATE occurs before a DELETE.
-    """
-    table_key = table.upper()
-    update_indices = [i for i, sql in enumerate(sql_list) if 'UPDATE' in sql.upper() and table_key in sql.upper()]
-    delete_indices = [i for i, sql in enumerate(sql_list) if 'DELETE' in sql.upper() and table_key in sql.upper()]
-    assert update_indices, f'Expected at least one UPDATE on {table}'
-    assert delete_indices, f'Expected at least one DELETE on {table}'
-    assert any(update_idx < delete_idx for update_idx in update_indices for delete_idx in delete_indices), (
-        'Expected at least one UPDATE to precede at least one DELETE'
-    )
-
-
 def generate_dummy_users():
     """
     Function to generate dummy users that needs to be retired