fix: Redact SSO PII before deletion

Author: ktyagiapphelix2u

openedx/core/djangoapps/user_api/accounts/signals.py

@@ -40,7 +40,7 @@ def redact_social_auth_pii_before_deletion(sender, instance, **kwargs):  # pylin
     Safety-net signal handler that redacts PII on any UserSocialAuth before deletion.
 
     Records deleted via ``redact_and_delete_social_auth`` will already be redacted;
-    this handler is a fallback for any other deletion path.
+    this handler is a fallback for any missed deletion path.
     """
     redacted_uid = get_redacted_social_auth_uid(instance.pk)
 

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

@@ -0,0 +1,70 @@
+"""
+Tests for user_api accounts signals.
+"""
+
+import logging
+from unittest.mock import patch
+
+from django.test import TestCase
+from social_django.models import UserSocialAuth
+
+from common.djangoapps.student.tests.factories import UserFactory
+from openedx.core.djangoapps.user_api.accounts.signals import get_redacted_social_auth_uid
+from openedx.core.djangolib.testing.utils import skip_unless_lms
+
+
+@skip_unless_lms
+class RedactSocialAuthPIIOnDeleteSignalTest(TestCase):
+    """
+    Tests for the redact_social_auth_pii_before_deletion pre_delete signal handler.
+    """
+
+    def setUp(self):
+        super().setUp()
+        self.user = UserFactory.create(username='testuser', email='testuser@example.com')
+
+    def _create_social_auth(self, uid='user@example.com', extra_data=None):
+        if extra_data is None:
+            extra_data = {'email': 'user@example.com', 'name': 'Test User'}
+        return UserSocialAuth.objects.create(
+            user=self.user,
+            provider='google-oauth2',
+            uid=uid,
+            extra_data=extra_data,
+        )
+
+    def test_signal_warns_and_redacts_when_not_already_redacted(self):
+        """
+        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(
+            '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):
+        """
+        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.
+        """
+        social_auth = self._create_social_auth()
+        social_auth.uid = get_redacted_social_auth_uid(social_auth.pk)
+        social_auth.extra_data = {}
+        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()
+
+        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

@@ -4,14 +4,16 @@
 import ddt
 from completion import models
 from completion.test_utils import CompletionWaffleTestMixin
+from django.db import connection
 from django.test import TestCase
-from django.test.utils import override_settings
+from django.test.utils import CaptureQueriesContext, 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.signals import get_redacted_social_auth_uid
 from openedx.core.djangoapps.user_api.accounts.utils import (
+    redact_and_delete_social_auth,
     retrieve_last_sitewide_block_completed,
 )
 from openedx.core.djangolib.testing.utils import skip_unless_lms
@@ -140,15 +142,30 @@ def test_retrieve_last_sitewide_block_completed(self):
 
 
 @skip_unless_lms
-class RedactUserSocialAuthPIITest(TestCase):
+class RedactAndDeleteSocialAuthTest(TestCase):
     """
-    Tests for SSO PII redaction before deletion.
+    Tests for the redact_and_delete_social_auth utility function.
     """
 
     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 at least one UPDATE on ``table`` precedes the DELETE in ``sql_list``.
+
+        This verifies that PII was redacted in the database before the row was removed,
+        which matters because downstream systems may use soft-deletes and could otherwise
+        retain unredacted data.
+        """
+        table_key = table.upper()
+        update_indices = [i for i, s in enumerate(sql_list) if 'UPDATE' in s.upper() and table_key in s.upper()]
+        delete_indices = [i for i, s in enumerate(sql_list) if 'DELETE' in s.upper() and table_key in s.upper()]
+        assert update_indices, f'Expected at least one UPDATE on {table}'
+        assert delete_indices, f'Expected at least one DELETE on {table}'
+        assert update_indices[0] < delete_indices[0], 'Expected UPDATE to precede DELETE'
+
     def create_social_auth(self, provider='google-oauth2', uid='user@example.com', extra_data=None):
         """
         Helper method to create UserSocialAuth instances for testing.
@@ -177,70 +194,23 @@ 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_delete_redacts_user_social_auth_pii(self):
+    def test_redact_and_delete_issues_update_before_delete(self):
         """
-        Test that deleting social auth redacts uid and extra_data before removal.
+        Test that redact_and_delete_social_auth issues an UPDATE (redaction) before the DELETE.
         """
         social_auth = self.create_social_auth()
         social_auth_id = social_auth.id
 
-        captured_states = []
+        with CaptureQueriesContext(connection) as ctx:
+            redact_and_delete_social_auth(self.user.id)
 
-        def capture_state_before_delete(sender, instance, **kwargs):  # pylint: disable=unused-argument
-            instance.refresh_from_db()
-            captured_states.append({
-                'id': instance.id,
-                'uid': instance.uid,
-                'extra_data': dict(instance.extra_data) if instance.extra_data else {},
-            })
-
-        from django.db.models.signals import pre_delete
-
-        pre_delete.connect(capture_state_before_delete, sender=UserSocialAuth)
-        try:
-            social_auth.delete()
-        finally:
-            pre_delete.disconnect(capture_state_before_delete, sender=UserSocialAuth)
-
-        assert captured_states == [{
-            'id': social_auth_id,
-            'uid': get_redacted_social_auth_uid(social_auth_id),
-            'extra_data': {},
-        }]
+        self._assert_update_before_delete([q['sql'] for q in ctx])
         assert not UserSocialAuth.objects.filter(id=social_auth_id).exists()
 
-    def test_delete_already_redacted_user_social_auth_is_idempotent(self):
+    def test_redact_and_delete_redacts_multiple_sso_providers(self):
         """
-        Test that deleting an already redacted social auth keeps the redacted state.
-        """
-        social_auth = self.create_social_auth()
-        social_auth.uid = get_redacted_social_auth_uid(social_auth.pk)
-        social_auth.extra_data = {}
-        social_auth.save(update_fields=['uid', 'extra_data'])
-        social_auth_id = social_auth.id
-
-        captured_states = []
-
-        def capture_state_before_delete(sender, instance, **kwargs):  # pylint: disable=unused-argument
-            instance.refresh_from_db()
-            captured_states.append((instance.uid, instance.extra_data))
-
-        from django.db.models.signals import pre_delete
-
-        pre_delete.connect(capture_state_before_delete, sender=UserSocialAuth)
-        try:
-            social_auth.delete()
-        finally:
-            pre_delete.disconnect(capture_state_before_delete, sender=UserSocialAuth)
-
-        assert captured_states == [
-            (get_redacted_social_auth_uid(social_auth_id), {}),
-        ]
-        assert not UserSocialAuth.objects.filter(id=social_auth_id).exists()
-
-    def test_delete_redacts_multiple_sso_providers(self):
-        """
-        Test that deletion redacts multiple SSO providers before removal.
+        Test that redact_and_delete_social_auth redacts and deletes records for
+        multiple SSO providers in a single call.
         """
         auths = [
             self.create_social_auth(
@@ -254,25 +224,11 @@ def test_delete_redacts_multiple_sso_providers(self):
                 extra_data={'email': 'saml@example.com', 'name': 'SAML User', 'uid': 'saml-uid'}
             ),
         ]
-        # Save IDs before deletion (they become None after delete)
         auth_ids = [auth.pk for auth in auths]
 
-        captured_states = []
-
-        def capture_state_before_delete(sender, instance, **kwargs):  # pylint: disable=unused-argument
-            instance.refresh_from_db()
-            captured_states.append((instance.provider, instance.uid, instance.extra_data))
-
-        from django.db.models.signals import pre_delete
-
-        pre_delete.connect(capture_state_before_delete, sender=UserSocialAuth)
-        try:
-            for auth in auths:
-                auth.delete()
-        finally:
-            pre_delete.disconnect(capture_state_before_delete, sender=UserSocialAuth)
+        with CaptureQueriesContext(connection) as ctx:
+            redact_and_delete_social_auth(self.user.id)
 
-        assert sorted(captured_states) == sorted([
-            ('google-oauth2', get_redacted_social_auth_uid(auth_ids[0]), {}),
-            ('tpa-saml', get_redacted_social_auth_uid(auth_ids[1]), {}),
-        ])
+        self._assert_update_before_delete([q['sql'] for q in ctx])
+        for auth_id in auth_ids:
+            assert not UserSocialAuth.objects.filter(id=auth_id).exists()

openedx/core/djangoapps/user_api/accounts/utils.py

@@ -206,16 +206,17 @@ def redact_and_delete_social_auth(user_id, skip_delete=False):
     """
     Redact PII from all UserSocialAuth records for the given user, then delete them.
 
-    Redaction happens before deletion so that any observers see only sanitised data.
     Downstream copies of data may use soft-deletes, and redacting before deleting
     ensures PII for retired users (or future retirements) is not retained.
-    The uid format matches ``get_redacted_social_auth_uid()``.
 
     ``skip_delete`` should only be set to True when called from the pre_delete signal
     handler, where deletion is already in progress.
+
+    The uid format matches ``get_redacted_social_auth_uid()``.
     """
     social_auth_queryset = UserSocialAuth.objects.filter(user_id=user_id)
     social_auth_queryset.update(
+        # Important: this redacted uid must match the format used by  ``get_redacted_social_auth_uid()``.
         uid=Concat(
             Value(REDACTED_SOCIAL_AUTH_UID_PREFIX),
             Cast('id', output_field=CharField()),

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

@@ -9,16 +9,20 @@
 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 django.db import connection
 from django.db.models.signals import pre_delete
+from django.test.utils import CaptureQueriesContext
 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
+    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
     setup_retirement_states,  # noqa: F401
 )
 from openedx.core.djangolib.testing.utils import skip_unless_lms  # lint-amnesty, pylint: disable=wrong-import-order
 
-from ...accounts.signals import get_redacted_social_auth_uid
 from ...models import UserRetirementStatus
 
 pytestmark = pytest.mark.django_db
@@ -117,8 +121,8 @@ def test_retire_user_redacts_sso_pii_before_deletion(setup_retirement_states):
     """
     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.
+    The safety-net pre_delete signal handler is disconnected for this test so that
+    we verify the redaction comes from retire_user itself, not the fallback signal.
     """
     user = UserFactory.create(username='sso-user', email='sso-user@example.com')
     social_auth = UserSocialAuth.objects.create(
@@ -133,32 +137,22 @@ def test_retire_user_redacts_sso_pii_before_deletion(setup_retirement_states):
     )
     social_auth_id = social_auth.id
 
-    captured_states = []
-
-    def capture_state_before_delete(sender, instance, **kwargs):  # pylint: disable=unused-argument
-        """Capture the database state seen by the pre_delete signal."""
-        instance.refresh_from_db()
-        captured_states.append({
-            'id': instance.id,
-            'uid': instance.uid,
-            'extra_data': dict(instance.extra_data) if instance.extra_data else {},
-        })
-
-    pre_delete.connect(capture_state_before_delete, sender=UserSocialAuth)
+    # Disconnect the safety-net signal so we prove retire_user itself redacts first.
+    pre_delete.disconnect(redact_social_auth_pii_before_deletion, sender=UserSocialAuth)
     try:
-        call_command('retire_user', username=user.username, user_email=user.email)
+        with CaptureQueriesContext(connection) as ctx:
+            call_command('retire_user', username=user.username, user_email=user.email)
     finally:
-        pre_delete.disconnect(capture_state_before_delete, sender=UserSocialAuth)
+        pre_delete.connect(redact_social_auth_pii_before_deletion, sender=UserSocialAuth)
 
-    # Verify that at the moment of deletion, the record was already redacted
-    assert captured_states == [{
-        'id': social_auth_id,
-        'uid': get_redacted_social_auth_uid(social_auth_id),
-        'extra_data': {},
-    }], \
-        "SSO records should be redacted before deletion"
+    sql_list = [q['sql'] for q in ctx]
+    table_key = 'SOCIAL_AUTH_USERSOCIALAUTH'
+    update_indices = [i for i, s in enumerate(sql_list) if 'UPDATE' in s.upper() and table_key in s.upper()]
+    delete_indices = [i for i, s in enumerate(sql_list) if 'DELETE' in s.upper() and table_key in s.upper()]
+    assert update_indices, 'Expected at least one UPDATE (redaction) on social_auth_usersocialauth'
+    assert delete_indices, 'Expected at least one DELETE on social_auth_usersocialauth'
+    assert update_indices[0] < delete_indices[0], 'Expected UPDATE (redaction) to precede DELETE'
 
-    # Verify deletion completed
     assert not UserSocialAuth.objects.filter(id=social_auth_id).exists()
 
     retired_user_status = UserRetirementStatus.objects.filter(original_username=user.username).first()
@@ -169,7 +163,10 @@ def capture_state_before_delete(sender, instance, **kwargs):  # pylint: disable=
 @skip_unless_lms
 def test_retire_user_redacts_each_social_auth_before_bulk_deletion(setup_retirement_states):  # lint-amnesty, pylint: disable=redefined-outer-name, unused-argument  # noqa: F811
     """
-    Test that each UserSocialAuth record is redacted before bulk deletion during retirement.
+    Test that all UserSocialAuth records are redacted before bulk deletion during retirement.
+
+    The safety-net pre_delete signal handler is disconnected for this test so that
+    we verify the redaction comes from retire_user itself, not the fallback signal.
     """
     user = UserFactory.create(username='multi-sso-user', email='multi-sso@example.com')
     google_auth = UserSocialAuth.objects.create(
@@ -184,25 +181,24 @@ def test_retire_user_redacts_each_social_auth_before_bulk_deletion(setup_retirem
         uid='saml-multi@example.com',
         extra_data={'email': 'saml-multi@example.com', 'name': 'SAML User', 'uid': 'saml-123'}
     )
-    # Save IDs before deletion (they become None after delete)
     google_auth_id = google_auth.id
     saml_auth_id = saml_auth.id
 
-    captured_states = []
-
-    def capture_state_before_delete(sender, instance, **kwargs):  # pylint: disable=unused-argument
-        """Capture the database state seen by the pre_delete signal."""
-        instance.refresh_from_db()
-        extra = dict(instance.extra_data) if instance.extra_data else {}
-        captured_states.append((instance.provider, instance.uid, extra))
-
-    pre_delete.connect(capture_state_before_delete, sender=UserSocialAuth)
+    # Disconnect the safety-net signal so we prove retire_user itself redacts first.
+    pre_delete.disconnect(redact_social_auth_pii_before_deletion, sender=UserSocialAuth)
     try:
-        call_command('retire_user', username=user.username, user_email=user.email)
+        with CaptureQueriesContext(connection) as ctx:
+            call_command('retire_user', username=user.username, user_email=user.email)
     finally:
-        pre_delete.disconnect(capture_state_before_delete, sender=UserSocialAuth)
-
-    assert sorted(captured_states) == sorted([
-        ('google-oauth2', get_redacted_social_auth_uid(google_auth_id), {}),
-        ('tpa-saml', get_redacted_social_auth_uid(saml_auth_id), {}),
-    ])
+        pre_delete.connect(redact_social_auth_pii_before_deletion, sender=UserSocialAuth)
+
+    sql_list = [q['sql'] for q in ctx]
+    table_key = 'SOCIAL_AUTH_USERSOCIALAUTH'
+    update_indices = [i for i, s in enumerate(sql_list) if 'UPDATE' in s.upper() and table_key in s.upper()]
+    delete_indices = [i for i, s in enumerate(sql_list) if 'DELETE' in s.upper() and table_key in s.upper()]
+    assert update_indices, 'Expected at least one UPDATE (redaction) on social_auth_usersocialauth'
+    assert delete_indices, 'Expected at least one DELETE on social_auth_usersocialauth'
+    assert update_indices[0] < delete_indices[0], 'Expected UPDATE (redaction) to precede DELETE'
+
+    assert not UserSocialAuth.objects.filter(id=google_auth_id).exists()
+    assert not UserSocialAuth.objects.filter(id=saml_auth_id).exists()