diff --git a/openedx/core/djangoapps/user_api/accounts/__init__.py b/openedx/core/djangoapps/user_api/accounts/__init__.py index faed01a6bb8d..e80aabe4169d 100644 --- a/openedx/core/djangoapps/user_api/accounts/__init__.py +++ b/openedx/core/djangoapps/user_api/accounts/__init__.py @@ -6,6 +6,9 @@ from django.utils.text import format_lazy from django.utils.translation import gettext_lazy as _ +# Import signals to ensure they are registered +from . import signals # noqa: F401, pylint: disable=unused-import + # The maximum length for the bio ("about me") account field BIO_MAX_LENGTH = 300 diff --git a/openedx/core/djangoapps/user_api/accounts/signals.py b/openedx/core/djangoapps/user_api/accounts/signals.py index be7bf75d99ee..d0f2df3a03fc 100644 --- a/openedx/core/djangoapps/user_api/accounts/signals.py +++ b/openedx/core/djangoapps/user_api/accounts/signals.py @@ -2,8 +2,24 @@ Django Signal related functionality for user_api accounts """ +import logging -from django.dispatch import Signal +from django.db.models.signals import pre_delete +from django.dispatch import Signal, receiver +from social_django.models import UserSocialAuth + +from .utils import REDACTED_SOCIAL_AUTH_UID_PREFIX, REDACTED_SOCIAL_AUTH_UID_SUFFIX, redact_and_delete_social_auth + +logger = logging.getLogger(__name__) + + +def get_redacted_social_auth_uid(pk): + """ + Return the redacted uid for a UserSocialAuth record. + + This must match the format used in redact_and_delete_social_auth. + """ + return f'{REDACTED_SOCIAL_AUTH_UID_PREFIX}{pk}{REDACTED_SOCIAL_AUTH_UID_SUFFIX}' # Signal to retire a user from LMS-initiated mailings (course mailings, etc) # providing_args=["user"] @@ -16,3 +32,24 @@ # Signal to retire LMS misc information # providing_args=["user"] USER_RETIRE_LMS_MISC = Signal() + + +@receiver(pre_delete, sender=UserSocialAuth) +def redact_social_auth_pii_before_deletion(sender, instance, **kwargs): # pylint: disable=unused-argument + """ + 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 missed deletion path. + """ + redacted_uid = get_redacted_social_auth_uid(instance.pk) + + # Safety-net in case the record wasn't redacted before delete. + if instance.extra_data or instance.uid != redacted_uid: + logger.warning( + 'Social auth link for user_id=%s, provider=%s was deleted without first being redacted.' + ' Redacting in pre_delete.', + instance.user_id, + instance.provider, + ) + redact_and_delete_social_auth(instance.user_id, skip_delete=True) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_signals.py b/openedx/core/djangoapps/user_api/accounts/tests/test_signals.py new file mode 100644 index 000000000000..7a6d6bd87fd5 --- /dev/null +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_signals.py @@ -0,0 +1,76 @@ +""" +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_get_redacted_social_auth_uid_format(self): + """ + Test that get_redacted_social_auth_uid returns the expected string format. + + This is the single source of truth for the redacted uid format. + """ + 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' + + @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 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) + + @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. + """ + 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 + + social_auth.delete() + + mock_redact.assert_not_called() + assert not UserSocialAuth.objects.filter(id=social_auth_id).exists() diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_utils.py b/openedx/core/djangoapps/user_api/accounts/tests/test_utils.py index 34eeee622811..c05f30d6870d 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_utils.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_utils.py @@ -1,15 +1,23 @@ """ Unit tests for custom UserProfile properties. """ +from contextlib import contextmanager import ddt from completion import models from completion.test_utils import CompletionWaffleTestMixin +from django.db import connection +from django.db.models.signals import pre_delete 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.utils import retrieve_last_sitewide_block_completed +from openedx.core.djangoapps.user_api.accounts.signals import redact_social_auth_pii_before_deletion +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 from xmodule.modulestore.tests.django_utils import ( SharedModuleStoreTestCase, # pylint: disable=wrong-import-order @@ -22,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.""" @@ -133,3 +174,71 @@ def test_retrieve_last_sitewide_block_completed(self): ) assert empty_block_url is None + + +@ddt.ddt +@skip_unless_lms +class RedactAndDeleteSocialAuthTest(TestCase): + """ + 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 create_social_auth(self, provider='google-oauth2', uid='user@example.com', extra_data=None): + """ + Helper method to create UserSocialAuth instances for testing. + """ + extra_data = extra_data or { + 'email': f'{provider}@example.com', + 'name': f'{provider.capitalize()} User', + 'id': '123456789', + } + return UserSocialAuth.objects.create( + user=self.user, + provider=provider, + uid=uid, + extra_data=extra_data, + ) + + def test_redact_and_delete_redacts_single_sso_record(self): + """ + Test that redact_and_delete_social_auth redacts and deletes a single SSO record. + """ + 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 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() + + 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() diff --git a/openedx/core/djangoapps/user_api/accounts/utils.py b/openedx/core/djangoapps/user_api/accounts/utils.py index d57107b97f11..083f45ecc9e0 100644 --- a/openedx/core/djangoapps/user_api/accounts/utils.py +++ b/openedx/core/djangoapps/user_api/accounts/utils.py @@ -11,6 +11,8 @@ from completion.models import BlockCompletion from completion.waffle import ENABLE_COMPLETION_TRACKING_SWITCH from django.conf import settings +from django.db.models import CharField, Value +from django.db.models.functions import Cast, Concat from django.utils.translation import gettext as _ from edx_django_utils.user import generate_password from social_django.models import UserSocialAuth @@ -23,6 +25,10 @@ from ..models import UserRetirementStatus +# Prefix and suffix used to build a per-record redacted uid for UserSocialAuth. +REDACTED_SOCIAL_AUTH_UID_PREFIX = 'redacted-before-delete-' +REDACTED_SOCIAL_AUTH_UID_SUFFIX = '@safe.com' + ENABLE_SECONDARY_EMAIL_FEATURE_SWITCH = 'enable_secondary_email_feature' LOGGER = logging.getLogger(__name__) @@ -196,6 +202,30 @@ def is_secondary_email_feature_enabled(): return waffle.switch_is_active(ENABLE_SECONDARY_EMAIL_FEATURE_SWITCH) +def redact_and_delete_social_auth(user_id, skip_delete=False): + """ + Redact PII from all UserSocialAuth records for the given user, then delete them. + + Downstream copies of data may use soft-deletes, and redacting before deleting + ensures PII for retired users (or future retirements) is not retained. + + ``skip_delete`` should only be set to True when called from the pre_delete signal + handler, where deletion is already in progress. + """ + social_auth_queryset = UserSocialAuth.objects.filter(user_id=user_id) + # Important: this redacted uid must match the format used by ``get_redacted_social_auth_uid()``. + social_auth_queryset.update( + uid=Concat( + Value(REDACTED_SOCIAL_AUTH_UID_PREFIX), + Cast('id', output_field=CharField()), + Value(REDACTED_SOCIAL_AUTH_UID_SUFFIX), + ), + extra_data={}, + ) + if not skip_delete: + social_auth_queryset.delete() + + def create_retirement_request_and_deactivate_account(user): """ Adds user to retirement queue, unlinks social auth accounts, changes user passwords @@ -204,8 +234,8 @@ def create_retirement_request_and_deactivate_account(user): # 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. + redact_and_delete_social_auth(user.id) # Change LMS password & email user.email = get_retired_email_by_email(user.email) diff --git a/openedx/core/djangoapps/user_api/management/commands/retire_user.py b/openedx/core/djangoapps/user_api/management/commands/retire_user.py index c022df829cba..3ce738b149c0 100644 --- a/openedx/core/djangoapps/user_api/management/commands/retire_user.py +++ b/openedx/core/djangoapps/user_api/management/commands/retire_user.py @@ -5,11 +5,11 @@ from django.contrib.auth.models import User # pylint: disable=imported-auth-user from django.core.management.base import BaseCommand, CommandError from django.db import transaction -from social_django.models import UserSocialAuth 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_and_delete_social_auth from ...models import BulkUserRetirementConfig, UserRetirementStatus logger = logging.getLogger(__name__) @@ -144,8 +144,8 @@ 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. + redact_and_delete_social_auth(user.id) # Change LMS password & email user.email = get_retired_email_by_email(user.email) user.set_unusable_password() diff --git a/openedx/core/djangoapps/user_api/management/tests/test_retire_user.py b/openedx/core/djangoapps/user_api/management/tests/test_retire_user.py index 8f1a61cfe48d..e57c1c50c938 100644 --- a/openedx/core/djangoapps/user_api/management/tests/test_retire_user.py +++ b/openedx/core/djangoapps/user_api/management/tests/test_retire_user.py @@ -5,15 +5,26 @@ import csv import os +from contextlib import contextmanager import pytest from django.contrib.auth.models import User # pylint: disable=imported-auth-user from django.core.management import CommandError, call_command - -from common.djangoapps.student.tests.factories import UserFactory # pylint: disable=wrong-import-order -from openedx.core.djangoapps.user_api.accounts.tests.retirement_helpers import ( # pylint: disable=unused-import, wrong-import-order +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 ( + redact_social_auth_pii_before_deletion, +) +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 @@ -21,6 +32,18 @@ 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(): + """ + Temporarily disconnect the fallback signal so these tests exercise the command 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) + def generate_dummy_users(): """ @@ -107,3 +130,44 @@ def test_retire_with_username_email_userfile(setup_retirement_states): # pylint 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 +@pytest.mark.parametrize('social_auth_configs', [ + # Single SSO provider + [ + {'provider': 'google-oauth2', 'uid': 'sso@example.com', + 'extra_data': {'email': 'sso@example.com', 'name': 'SSO Test User', 'id': '123456789'}}, + ], + # Multiple SSO providers + [ + {'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-123'}}, + ], +]) +def test_retire_user_redacts_sso_pii_before_deletion(setup_retirement_states, social_auth_configs): # lint-amnesty, pylint: disable=redefined-outer-name, unused-argument # noqa: F811 + """ + Test that SSO PII is redacted before UserSocialAuth records are deleted during retirement. + Covers both single and multiple SSO provider scenarios. + + The safety-net pre_delete signal handler is disconnected so we verify the redaction + comes from retire_user itself, not the fallback signal. + """ + user = UserFactory.create(username='sso-user', email='sso-user@example.com') + auth_ids = [ + UserSocialAuth.objects.create(user=user, **cfg).id + for cfg in social_auth_configs + ] + + 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]) + for auth_id in auth_ids: + assert not UserSocialAuth.objects.filter(id=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'