diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 44f26de7..741f59fa 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -12,7 +12,16 @@ Change Log Unreleased ---------- -* + +[4.6.0] - 2025-06-18 +-------------------- + +Changed +~~~~~~~ + +* Improved test coverage by replacing MagicMock with real load_strategy() implementation. +* Fixed email update handling in authentication pipeline. +* Added logging for email updates. Added ~~~~~~~ diff --git a/auth_backends/__init__.py b/auth_backends/__init__.py index 86a49232..aa7c8494 100644 --- a/auth_backends/__init__.py +++ b/auth_backends/__init__.py @@ -3,4 +3,4 @@ These package is designed to be used primarily with Open edX Django projects, but should be compatible with non-edX projects as well. """ -__version__ = '4.5.0' # pragma: no cover +__version__ = '4.6.0' # pragma: no cover diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index 3e67c55b..d92259e6 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -1,12 +1,27 @@ """Python-social-auth pipeline functions. -For more info visit http://python-social-auth.readthedocs.org/en/latest/pipeline.html. +For more info visit https://python-social-auth.readthedocs.io/en/latest/pipeline.html. """ +import logging from django.contrib.auth import get_user_model +from edx_toggles.toggles import SettingToggle +from edx_django_utils.monitoring import set_custom_attribute - +logger = logging.getLogger(__name__) User = get_user_model() +# .. toggle_name: SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH +# .. toggle_implementation: SettingToggle +# .. toggle_default: False +# .. toggle_description: Determines whether to block email updates when usernames don't match. +# When enabled (True), email updates will be blocked when the username in social auth details +# doesn't match the user's username. When disabled (False), email updates will proceed regardless +# of username mismatches. This will be used for a temporary rollout. +# .. toggle_use_cases: temporary +# .. toggle_creation_date: 2025-06-18 +# .. toggle_target_removal_date: 2025-08-18 +SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH = SettingToggle("SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH", default=False) + # pylint: disable=unused-argument # The function parameters must be named exactly as they are below. @@ -33,9 +48,63 @@ def get_user_if_exists(strategy, details, user=None, *args, **kwargs): # pylint def update_email(strategy, details, user=None, *args, **kwargs): # pylint: disable=keyword-arg-before-vararg """Update the user's email address using data from provider.""" + if user: - email = details.get('email') + # Get usernames for comparison, using defensive coding + details_username = details.get('username') + user_username = getattr(user, 'username', None) + # Check if usernames don't match + username_mismatch = details_username != user_username + + # .. custom_attribute_name: update_email.username_mismatch + # .. custom_attribute_description: Tracks whether there's a mismatch between + # the username in the social details and the user's actual username. + # True if usernames don't match, False if they match. + set_custom_attribute('update_email.username_mismatch', username_mismatch) + + # .. custom_attribute_name: update_email.rollout_toggle_enabled + # .. custom_attribute_description: Tracks whether the SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH + # toggle is enabled during this pipeline execution. + set_custom_attribute('update_email.rollout_toggle_enabled', SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled()) + if username_mismatch: + # Log warning and set additional custom attributes for mismatches + logger.warning( + "Username mismatch during email update. User username: %s, Details username: %s", + user_username, + details_username + ) + # .. custom_attribute_name: update_email.details_username + # .. custom_attribute_description: Records the username provided in the + # social details when a mismatch occurs with the user's username. + set_custom_attribute('update_email.details_username', details_username) + + # .. custom_attribute_name: update_email.user_username + # .. custom_attribute_description: Records the actual username of the user + # when a mismatch occurs with the social details username. + set_custom_attribute('update_email.user_username', user_username) + + # .. custom_attribute_name: update_email.details_has_email + # .. custom_attribute_description: Records whether the details contain an email + # when a username mismatch occurs, to identify potential edge cases. + set_custom_attribute('update_email.details_has_email', bool(details.get('email'))) + + # Only exit if the toggle is enabled + if SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled(): + logger.warning( + "Skipping email update for user %s due to username mismatch and " + "SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH toggle enabled", + user_username + ) + return # Exit without updating email + + # Proceed with email update only if usernames match or toggle is disabled + email = details.get('email') if email and user.email != email: user.email = email strategy.storage.user.changed(user) + + # .. custom_attribute_name: update_email.email_updated + # .. custom_attribute_description: Indicates that the user's email was + # actually updated during this pipeline execution. + set_custom_attribute('update_email.email_updated', True) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 26f5f4a0..060dcb30 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -1,5 +1,6 @@ """ Tests for pipelines. """ +from unittest.mock import patch from django.contrib.auth import get_user_model from django.test import TestCase from social_django.utils import load_strategy @@ -40,20 +41,124 @@ class UpdateEmailPipelineTests(TestCase): def setUp(self): super().setUp() - self.user = User.objects.create() + self.user = User.objects.create(username='test_user') + self.strategy = load_strategy() - def test_update_email(self): - """ Verify that user email is updated upon changing email. """ + @patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled') + @patch('auth_backends.pipeline.set_custom_attribute') + def test_update_email(self, mock_set_attribute, mock_toggle): + """ Verify that user email is updated upon changing email when usernames match. """ + mock_toggle.return_value = False updated_email = 'updated@example.com' self.assertNotEqual(self.user.email, updated_email) - update_email(load_strategy(), {'email': updated_email}, user=self.user) - self.user = User.objects.get(username=self.user.username) - self.assertEqual(self.user.email, updated_email) + initial_email = self.user.email - def test_update_email_with_none(self): + update_email(self.strategy, {'email': updated_email, 'username': 'test_user'}, user=self.user) + + updated_user = User.objects.get(pk=self.user.pk) + self.assertEqual(updated_user.email, updated_email) + self.assertNotEqual(updated_user.email, initial_email) + + mock_set_attribute.assert_any_call('update_email.username_mismatch', False) + mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False) + self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=True) + + @patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled') + @patch('auth_backends.pipeline.set_custom_attribute') + def test_update_email_with_none(self, mock_set_attribute, mock_toggle): """ Verify that user email is not updated if email value is None. """ + mock_toggle.return_value = False old_email = self.user.email - update_email(load_strategy(), {'email': None}, user=self.user) - self.user = User.objects.get(username=self.user.username) - self.assertEqual(self.user.email, old_email) + + update_email(self.strategy, {'email': None, 'username': 'test_user'}, user=self.user) + + updated_user = User.objects.get(pk=self.user.pk) + self.assertEqual(updated_user.email, old_email) + + mock_set_attribute.assert_any_call('update_email.username_mismatch', False) + mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False) + self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=False) + + @patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled') + @patch('auth_backends.pipeline.logger') + @patch('auth_backends.pipeline.set_custom_attribute') + def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mock_logger, mock_toggle): + """ Verify that email is not updated when usernames don't match and toggle is enabled. """ + mock_toggle.return_value = True + + old_email = self.user.email + updated_email = 'updated@example.com' + + update_email(self.strategy, {'email': updated_email, 'username': 'different_user'}, user=self.user) + + updated_user = User.objects.get(pk=self.user.pk) + self.assertEqual(updated_user.email, old_email) + + self.assertEqual(mock_logger.warning.call_count, 2) + mock_logger.warning.assert_any_call( + "Username mismatch during email update. User username: %s, Details username: %s", + 'test_user', 'different_user' + ) + mock_logger.warning.assert_any_call( + "Skipping email update for user %s due to username mismatch and " + "SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH toggle enabled", + 'test_user' + ) + + mock_set_attribute.assert_any_call('update_email.username_mismatch', True) + mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', True) + mock_set_attribute.assert_any_call('update_email.details_username', 'different_user') + mock_set_attribute.assert_any_call('update_email.user_username', 'test_user') + mock_set_attribute.assert_any_call('update_email.details_has_email', True) + self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=False) + + @patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled') + @patch('auth_backends.pipeline.logger') + @patch('auth_backends.pipeline.set_custom_attribute') + def test_username_mismatch_with_update_toggle_disabled(self, mock_set_attribute, mock_logger, mock_toggle): + """ Verify that email is updated when usernames don't match but toggle is disabled. """ + mock_toggle.return_value = False + + old_email = self.user.email + updated_email = 'updated@example.com' + + update_email(self.strategy, {'email': updated_email, 'username': 'different_user'}, user=self.user) + + updated_user = User.objects.get(pk=self.user.pk) + self.assertEqual(updated_user.email, updated_email) + self.assertNotEqual(updated_user.email, old_email) + + mock_logger.warning.assert_called_once() + + mock_set_attribute.assert_any_call('update_email.username_mismatch', True) + mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False) + mock_set_attribute.assert_any_call('update_email.details_username', 'different_user') + mock_set_attribute.assert_any_call('update_email.user_username', 'test_user') + mock_set_attribute.assert_any_call('update_email.details_has_email', True) + self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=True) + + def assert_attribute_was_set(self, mock_set_attribute, attribute_name, should_exist=True): + """ + Assert that a specific attribute was or was not set via set_custom_attribute. + + Args: + mock_set_attribute: The mocked set_custom_attribute function + attribute_name: The name of the attribute to check + should_exist: If True, assert the attribute was set; if False, assert it wasn't + """ + matching_calls = [ + call for call in mock_set_attribute.call_args_list + if call[0][0] == attribute_name + ] + + if should_exist: + self.assertGreater( + len(matching_calls), 0, + f"Expected '{attribute_name}' to be set, but it wasn't" + ) + else: + self.assertEqual( + len(matching_calls), 0, + f"Expected '{attribute_name}' not to be set, but it was: {matching_calls}" + ) diff --git a/requirements/base.in b/requirements/base.in index 5bf6a159..4494e725 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -2,6 +2,8 @@ -c constraints.txt Django +edx-django-utils +edx-toggles pyjwt[crypto]>=2.1.0 # depends on newer jwt.decode and jwt.encode functionality six social-auth-app-django diff --git a/requirements/base.txt b/requirements/base.txt index 444ca8bd..9ad9ed08 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -6,13 +6,21 @@ # asgiref==3.8.1 # via django -certifi==2025.1.31 +certifi==2025.4.26 # via requests cffi==1.17.1 - # via cryptography -charset-normalizer==3.4.1 + # via + # cryptography + # pynacl +charset-normalizer==3.4.2 # via requests -cryptography==44.0.2 +click==8.2.1 + # via + # code-annotations + # edx-django-utils +code-annotations==2.3.0 + # via edx-toggles +cryptography==45.0.3 # via # pyjwt # social-auth-core @@ -20,25 +28,57 @@ defusedxml==0.7.1 # via # python3-openid # social-auth-core -django==4.2.20 +django==4.2.22 # via # -c requirements/common_constraints.txt # -r requirements/base.in + # django-crum + # django-waffle + # edx-django-utils + # edx-toggles # social-auth-app-django +django-crum==0.7.9 + # via + # edx-django-utils + # edx-toggles +django-waffle==4.2.0 + # via + # edx-django-utils + # edx-toggles +edx-django-utils==8.0.0 + # via + # -r requirements/base.in + # edx-toggles +edx-toggles==5.3.0 + # via -r requirements/base.in idna==3.10 # via requests +jinja2==3.1.6 + # via code-annotations +markupsafe==3.0.2 + # via jinja2 oauthlib==3.2.2 # via # requests-oauthlib # social-auth-core +pbr==6.1.1 + # via stevedore +psutil==7.0.0 + # via edx-django-utils pycparser==2.22 # via cffi pyjwt[crypto]==2.10.1 # via # -r requirements/base.in # social-auth-core +pynacl==1.5.0 + # via edx-django-utils +python-slugify==8.0.4 + # via code-annotations python3-openid==3.2.0 # via social-auth-core +pyyaml==6.0.2 + # via code-annotations requests==2.32.3 # via # requests-oauthlib @@ -49,13 +89,22 @@ six==1.17.0 # via -r requirements/base.in social-auth-app-django==5.4.3 # via -r requirements/base.in -social-auth-core==4.5.6 +social-auth-core==4.6.1 # via # -r requirements/base.in # social-auth-app-django sqlparse==0.5.3 # via django +stevedore==5.4.1 + # via + # code-annotations + # edx-django-utils +text-unidecode==1.3 + # via python-slugify urllib3==2.2.3 # via # -c requirements/common_constraints.txt # requests + +# The following packages are considered to be unsafe in a requirements file: +# setuptools