From 68fbf1007463ba291798734441b4f6382ae2ffa6 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Tue, 3 Jun 2025 14:59:23 +0530 Subject: [PATCH 01/12] fix: bug in third party authentication email and name update --- auth_backends/pipeline.py | 35 +++++++++++++++- auth_backends/tests/test_pipeline.py | 61 ++++++++++++++++++++++++---- requirements/base.in | 1 + 3 files changed, 89 insertions(+), 8 deletions(-) diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index 3e67c55b..c14ebf40 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -2,9 +2,12 @@ For more info visit http://python-social-auth.readthedocs.org/en/latest/pipeline.html. """ +import logging from django.contrib.auth import get_user_model +from edx_django_utils.monitoring import set_custom_attribute +logger = logging.getLogger(__name__) User = get_user_model() @@ -34,8 +37,38 @@ 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 match + username_match = 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 authentication details and the user's actual username. + # True if usernames don't match, False if they match. + set_custom_attribute('update_email.username_mismatch', not username_match) + + if not username_match: + # 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 + # authentication details when a mismatch occurs with the user's username. + set_custom_attribute('update_email.details_username', str(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 authentication details username. + set_custom_attribute('update_email.user_username', str(user_username)) + return # Exit without updating email + + # Proceed with email update only if usernames match + email = details.get('email') if email and user.email != email: user.email = email strategy.storage.user.changed(user) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 26f5f4a0..440d0885 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -1,8 +1,8 @@ """ Tests for pipelines. """ +from unittest.mock import patch, MagicMock from django.contrib.auth import get_user_model from django.test import TestCase -from social_django.utils import load_strategy from auth_backends.pipeline import get_user_if_exists, update_email @@ -40,20 +40,67 @@ class UpdateEmailPipelineTests(TestCase): def setUp(self): super().setUp() - self.user = User.objects.create() + self.user = User.objects.create(username='test_user') + self.strategy = MagicMock() - def test_update_email(self): - """ Verify that user email is updated upon changing email. """ + # Make strategy.storage.user.changed actually save the user + def save_user(user): + user.save() + + self.strategy.storage.user.changed.side_effect = save_user + + @patch('auth_backends.pipeline.set_custom_attribute') + def test_update_email(self, mock_set_attribute): + """ Verify that user email is updated upon changing email when usernames match. """ updated_email = 'updated@example.com' self.assertNotEqual(self.user.email, updated_email) - update_email(load_strategy(), {'email': updated_email}, user=self.user) + # Provide matching username in details + update_email(self.strategy, {'email': updated_email, 'username': 'test_user'}, user=self.user) + + # Verify email was updated self.user = User.objects.get(username=self.user.username) self.assertEqual(self.user.email, updated_email) + self.strategy.storage.user.changed.assert_called_once_with(self.user) + + # Verify custom attribute was set correctly + mock_set_attribute.assert_called_with('update_email.username_mismatch', False) - def test_update_email_with_none(self): + @patch('auth_backends.pipeline.set_custom_attribute') + def test_update_email_with_none(self, mock_set_attribute): """ Verify that user email is not updated if email value is None. """ old_email = self.user.email - update_email(load_strategy(), {'email': None}, user=self.user) + + # Provide matching username in details + update_email(self.strategy, {'email': None, 'username': 'test_user'}, user=self.user) + + # Verify email was not updated + self.user = User.objects.get(username=self.user.username) + self.assertEqual(self.user.email, old_email) + self.strategy.storage.user.changed.assert_not_called() + + # Verify custom attribute was still set + mock_set_attribute.assert_called_with('update_email.username_mismatch', False) + + @patch('auth_backends.pipeline.logger') + @patch('auth_backends.pipeline.set_custom_attribute') + def test_username_mismatch_no_update(self, mock_set_attribute, mock_logger): + """ Verify that email is not updated when usernames don't match. """ + old_email = self.user.email + updated_email = 'updated@example.com' + + # Provide mismatched username in details + update_email(self.strategy, {'email': updated_email, 'username': 'different_user'}, user=self.user) + + # Verify email was not updated self.user = User.objects.get(username=self.user.username) self.assertEqual(self.user.email, old_email) + self.strategy.storage.user.changed.assert_not_called() + + # Verify logger was called with warning + mock_logger.warning.assert_called_once() + + # Verify all custom attributes were set correctly + mock_set_attribute.assert_any_call('update_email.username_mismatch', 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') diff --git a/requirements/base.in b/requirements/base.in index 5bf6a159..722fe986 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -6,3 +6,4 @@ pyjwt[crypto]>=2.1.0 # depends on newer jwt.decode and jwt.encode function six social-auth-app-django social-auth-core # Common auth interfaces +edx-django-utils \ No newline at end of file From 17019a251417af71306c5983714d63ce261fec2b Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 9 Jun 2025 10:16:03 +0000 Subject: [PATCH 02/12] feat: update version to 4.6.0 and enhance email update logic with toggle support --- auth_backends/__init__.py | 2 +- auth_backends/pipeline.py | 39 +++++++++++++++++++++++----- auth_backends/tests/test_pipeline.py | 36 +++++++++++++++++++++++-- requirements/base.in | 2 +- requirements/base.txt | 36 ++++++++++++++++++++----- 5 files changed, 98 insertions(+), 17 deletions(-) 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 c14ebf40..dbfacedb 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -1,15 +1,28 @@ """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. +# .. toggle_use_cases: temporary +# .. toggle_creation_date: 2025-06-05 +# .. toggle_warnings: This is a temporary toggle for a security enhancement rollout. +# .. toggle_tickets: ARCHBOM-2181 +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. @@ -45,7 +58,7 @@ def update_email(strategy, details, user=None, *args, **kwargs): # pylint: disa # .. custom_attribute_name: update_email.username_mismatch # .. custom_attribute_description: Tracks whether there's a mismatch between - # the username in the authentication details and the user's actual username. + # 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', not username_match) @@ -58,16 +71,28 @@ def update_email(strategy, details, user=None, *args, **kwargs): # pylint: disa ) # .. custom_attribute_name: update_email.details_username # .. custom_attribute_description: Records the username provided in the - # authentication details when a mismatch occurs with the user's username. + # social details when a mismatch occurs with the user's username. set_custom_attribute('update_email.details_username', str(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 authentication details username. + # when a mismatch occurs with the social details username. set_custom_attribute('update_email.user_username', str(user_username)) - return # Exit without updating email + + # .. 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', details.get('email') is not None) + + # 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 + # 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 diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 440d0885..1cd4ffa5 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -82,10 +82,14 @@ def test_update_email_with_none(self, mock_set_attribute): # Verify custom attribute was still set mock_set_attribute.assert_called_with('update_email.username_mismatch', 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(self, mock_set_attribute, mock_logger): - """ Verify that email is not updated when usernames don't match. """ + 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. """ + # Configure toggle to be enabled + mock_toggle.return_value = True + old_email = self.user.email updated_email = 'updated@example.com' @@ -104,3 +108,31 @@ def test_username_mismatch_no_update(self, mock_set_attribute, mock_logger): mock_set_attribute.assert_any_call('update_email.username_mismatch', 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) + + @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. """ + # Configure toggle to be disabled + mock_toggle.return_value = False + + updated_email = 'updated@example.com' + + # Provide mismatched username in details + update_email(self.strategy, {'email': updated_email, 'username': 'different_user'}, user=self.user) + + # Verify email was updated despite username mismatch + self.user = User.objects.get(username=self.user.username) + self.assertEqual(self.user.email, updated_email) + self.strategy.storage.user.changed.assert_called_once_with(self.user) + + # Verify logger was still called with warning + mock_logger.warning.assert_called_once() + + # Verify all custom attributes were set correctly + mock_set_attribute.assert_any_call('update_email.username_mismatch', 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) diff --git a/requirements/base.in b/requirements/base.in index 722fe986..cad9f0fe 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -2,8 +2,8 @@ -c constraints.txt Django +edx-django-utils pyjwt[crypto]>=2.1.0 # depends on newer jwt.decode and jwt.encode functionality six social-auth-app-django social-auth-core # Common auth interfaces -edx-django-utils \ No newline at end of file diff --git a/requirements/base.txt b/requirements/base.txt index 444ca8bd..c1902da5 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -6,13 +6,17 @@ # 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 edx-django-utils +cryptography==45.0.3 # via # pyjwt # social-auth-core @@ -20,23 +24,38 @@ 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 # social-auth-app-django +django-crum==0.7.9 + # via edx-django-utils +django-waffle==4.2.0 + # via edx-django-utils +edx-django-utils==8.0.0 + # via -r requirements/base.in idna==3.10 # via requests 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 python3-openid==3.2.0 # via social-auth-core requests==2.32.3 @@ -49,13 +68,18 @@ 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 edx-django-utils urllib3==2.2.3 # via # -c requirements/common_constraints.txt # requests + +# The following packages are considered to be unsafe in a requirements file: +# setuptools From 55a1470aa1f91ad8f0ef0c8c4ec4f04181e3ce25 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 9 Jun 2025 10:34:07 +0000 Subject: [PATCH 03/12] feat: add edx-toggles to requirements and update dependency comments --- requirements/base.in | 1 + requirements/base.txt | 33 +++++++++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/requirements/base.in b/requirements/base.in index cad9f0fe..4494e725 100644 --- a/requirements/base.in +++ b/requirements/base.in @@ -3,6 +3,7 @@ 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 c1902da5..9ad9ed08 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -15,7 +15,11 @@ cffi==1.17.1 charset-normalizer==3.4.2 # via requests click==8.2.1 - # via edx-django-utils + # via + # code-annotations + # edx-django-utils +code-annotations==2.3.0 + # via edx-toggles cryptography==45.0.3 # via # pyjwt @@ -31,15 +35,28 @@ django==4.2.22 # django-crum # django-waffle # edx-django-utils + # edx-toggles # social-auth-app-django django-crum==0.7.9 - # via edx-django-utils + # via + # edx-django-utils + # edx-toggles django-waffle==4.2.0 - # via edx-django-utils + # 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 @@ -56,8 +73,12 @@ pyjwt[crypto]==2.10.1 # 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 @@ -75,7 +96,11 @@ social-auth-core==4.6.1 sqlparse==0.5.3 # via django stevedore==5.4.1 - # via edx-django-utils + # via + # code-annotations + # edx-django-utils +text-unidecode==1.3 + # via python-slugify urllib3==2.2.3 # via # -c requirements/common_constraints.txt From b9655b068c4eb9179f9fe987338d533e15218f4c Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 9 Jun 2025 10:41:00 +0000 Subject: [PATCH 04/12] fix: improve logging message formatting in update_email function and clean up test cases --- auth_backends/pipeline.py | 3 ++- auth_backends/tests/test_pipeline.py | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index dbfacedb..a3c08078 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -87,7 +87,8 @@ def update_email(strategy, details, user=None, *args, **kwargs): # pylint: disa # 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", + "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 diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 1cd4ffa5..36097092 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -89,7 +89,7 @@ def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mo """ Verify that email is not updated when usernames don't match and toggle is enabled. """ # Configure toggle to be enabled mock_toggle.return_value = True - + old_email = self.user.email updated_email = 'updated@example.com' @@ -117,7 +117,7 @@ def test_username_mismatch_with_update_toggle_disabled(self, mock_set_attribute, """ Verify that email is updated when usernames don't match but toggle is disabled. """ # Configure toggle to be disabled mock_toggle.return_value = False - + updated_email = 'updated@example.com' # Provide mismatched username in details From ac6a481337aaa28a50e35ad570655ae51e96a391 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 9 Jun 2025 10:44:05 +0000 Subject: [PATCH 05/12] fix: fixed lint errors in pipeline.py file. --- auth_backends/pipeline.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index a3c08078..a4de2baa 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -78,12 +78,12 @@ def update_email(strategy, details, user=None, *args, **kwargs): # pylint: disa # .. 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', str(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', details.get('email') is not None) - + # Only exit if the toggle is enabled if SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled(): logger.warning( From 49e3b35901c7ce716f0d9c84df86cfdf89d68ecb Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 9 Jun 2025 11:01:08 +0000 Subject: [PATCH 06/12] fix: enhance logging verification for username mismatch in update_email tests --- auth_backends/tests/test_pipeline.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 36097092..c8217964 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -101,8 +101,17 @@ def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mo self.assertEqual(self.user.email, old_email) self.strategy.storage.user.changed.assert_not_called() - # Verify logger was called with warning - mock_logger.warning.assert_called_once() + # Verify logger was called with both warnings + 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' + ) # Verify all custom attributes were set correctly mock_set_attribute.assert_any_call('update_email.username_mismatch', True) From 3be8be1cd0cb83693fd5096fb928481dc1b2ba43 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 9 Jun 2025 11:06:17 +0000 Subject: [PATCH 07/12] fix: add target removal date for SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH toggle --- auth_backends/pipeline.py | 1 + 1 file changed, 1 insertion(+) diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index a4de2baa..81895480 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -19,6 +19,7 @@ # of username mismatches. # .. toggle_use_cases: temporary # .. toggle_creation_date: 2025-06-05 +# .. toggle_target_removal_date: 2026-06-05 # .. toggle_warnings: This is a temporary toggle for a security enhancement rollout. # .. toggle_tickets: ARCHBOM-2181 SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH = SettingToggle("SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH", default=False) From 933a0dcd445551444e54dd8a7ae6f6e715d97572 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Thu, 12 Jun 2025 09:23:47 +0000 Subject: [PATCH 08/12] fix: update CHANGELOG for version 4.6.0 and improve test coverage in update_email pipeline --- CHANGELOG.rst | 9 +++++ auth_backends/pipeline.py | 8 ++--- auth_backends/tests/test_pipeline.py | 52 +++++++++------------------- 3 files changed, 28 insertions(+), 41 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 44f26de7..8d5a3e89 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,15 @@ Unreleased ---------- * +[4.6.0] - 2025-06-09 +-------------------- + +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/pipeline.py b/auth_backends/pipeline.py index 81895480..b63e6eca 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -16,12 +16,10 @@ # .. 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. +# of username mismatches.This will be used for a temporary rollout. # .. toggle_use_cases: temporary # .. toggle_creation_date: 2025-06-05 # .. toggle_target_removal_date: 2026-06-05 -# .. toggle_warnings: This is a temporary toggle for a security enhancement rollout. -# .. toggle_tickets: ARCHBOM-2181 SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH = SettingToggle("SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH", default=False) @@ -73,12 +71,12 @@ def update_email(strategy, details, user=None, *args, **kwargs): # pylint: disa # .. 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', str(details_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', str(user_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 diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index c8217964..530e6a4b 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -1,8 +1,9 @@ """ Tests for pipelines. """ -from unittest.mock import patch, MagicMock +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 from auth_backends.pipeline import get_user_if_exists, update_email @@ -41,13 +42,7 @@ class UpdateEmailPipelineTests(TestCase): def setUp(self): super().setUp() self.user = User.objects.create(username='test_user') - self.strategy = MagicMock() - - # Make strategy.storage.user.changed actually save the user - def save_user(user): - user.save() - - self.strategy.storage.user.changed.side_effect = save_user + self.strategy = load_strategy() @patch('auth_backends.pipeline.set_custom_attribute') def test_update_email(self, mock_set_attribute): @@ -55,15 +50,14 @@ def test_update_email(self, mock_set_attribute): updated_email = 'updated@example.com' self.assertNotEqual(self.user.email, updated_email) - # Provide matching username in details + initial_email = self.user.email + update_email(self.strategy, {'email': updated_email, 'username': 'test_user'}, user=self.user) - # Verify email was updated - self.user = User.objects.get(username=self.user.username) - self.assertEqual(self.user.email, updated_email) - self.strategy.storage.user.changed.assert_called_once_with(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) - # Verify custom attribute was set correctly mock_set_attribute.assert_called_with('update_email.username_mismatch', False) @patch('auth_backends.pipeline.set_custom_attribute') @@ -71,15 +65,11 @@ def test_update_email_with_none(self, mock_set_attribute): """ Verify that user email is not updated if email value is None. """ old_email = self.user.email - # Provide matching username in details update_email(self.strategy, {'email': None, 'username': 'test_user'}, user=self.user) - # Verify email was not updated - self.user = User.objects.get(username=self.user.username) - self.assertEqual(self.user.email, old_email) - self.strategy.storage.user.changed.assert_not_called() + updated_user = User.objects.get(pk=self.user.pk) + self.assertEqual(updated_user.email, old_email) - # Verify custom attribute was still set mock_set_attribute.assert_called_with('update_email.username_mismatch', False) @patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled') @@ -87,21 +77,16 @@ def test_update_email_with_none(self, mock_set_attribute): @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. """ - # Configure toggle to be enabled mock_toggle.return_value = True old_email = self.user.email updated_email = 'updated@example.com' - # Provide mismatched username in details update_email(self.strategy, {'email': updated_email, 'username': 'different_user'}, user=self.user) - # Verify email was not updated - self.user = User.objects.get(username=self.user.username) - self.assertEqual(self.user.email, old_email) - self.strategy.storage.user.changed.assert_not_called() + updated_user = User.objects.get(pk=self.user.pk) + self.assertEqual(updated_user.email, old_email) - # Verify logger was called with both warnings 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", @@ -113,7 +98,6 @@ def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mo 'test_user' ) - # Verify all custom attributes were set correctly mock_set_attribute.assert_any_call('update_email.username_mismatch', 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') @@ -124,23 +108,19 @@ def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mo @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. """ - # Configure toggle to be disabled mock_toggle.return_value = False + old_email = self.user.email updated_email = 'updated@example.com' - # Provide mismatched username in details update_email(self.strategy, {'email': updated_email, 'username': 'different_user'}, user=self.user) - # Verify email was updated despite username mismatch - self.user = User.objects.get(username=self.user.username) - self.assertEqual(self.user.email, updated_email) - self.strategy.storage.user.changed.assert_called_once_with(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) - # Verify logger was still called with warning mock_logger.warning.assert_called_once() - # Verify all custom attributes were set correctly mock_set_attribute.assert_any_call('update_email.username_mismatch', 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') From 6435564f096567ede177265d7042e823a5659e8a Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Thu, 12 Jun 2025 09:27:37 +0000 Subject: [PATCH 09/12] fix: remove unnecessary blank line in update_email test case --- auth_backends/tests/test_pipeline.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 530e6a4b..6acc8f43 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -51,7 +51,7 @@ def test_update_email(self, mock_set_attribute): self.assertNotEqual(self.user.email, updated_email) initial_email = self.user.email - + update_email(self.strategy, {'email': updated_email, 'username': 'test_user'}, user=self.user) updated_user = User.objects.get(pk=self.user.pk) From 0ca90582f1d74bcb5b68bcfc82ba7a83d4047687 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Tue, 17 Jun 2025 06:48:14 +0000 Subject: [PATCH 10/12] fix: update CHANGELOG for version 4.6.0 and adjust email update pipeline tests for username mismatch toggle --- CHANGELOG.rst | 4 ++-- auth_backends/pipeline.py | 27 +++++++++++++++++++-------- auth_backends/tests/test_pipeline.py | 20 ++++++++++++++++---- 3 files changed, 37 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 8d5a3e89..741f59fa 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -12,8 +12,8 @@ Change Log Unreleased ---------- -* -[4.6.0] - 2025-06-09 + +[4.6.0] - 2025-06-18 -------------------- Changed diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index b63e6eca..d92259e6 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -16,10 +16,10 @@ # .. 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. +# of username mismatches. This will be used for a temporary rollout. # .. toggle_use_cases: temporary -# .. toggle_creation_date: 2025-06-05 -# .. toggle_target_removal_date: 2026-06-05 +# .. 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) @@ -48,20 +48,26 @@ 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: # Get usernames for comparison, using defensive coding details_username = details.get('username') user_username = getattr(user, 'username', None) - # Check if usernames match - username_match = details_username == user_username + # 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', not username_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 not username_match: + 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", @@ -81,7 +87,7 @@ def update_email(strategy, details, user=None, *args, **kwargs): # pylint: disa # .. 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', details.get('email') is not None) + 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(): @@ -97,3 +103,8 @@ def update_email(strategy, details, user=None, *args, **kwargs): # pylint: disa 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 6acc8f43..68f6d3fd 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -44,9 +44,11 @@ def setUp(self): self.user = User.objects.create(username='test_user') self.strategy = load_strategy() + @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): + 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) @@ -58,11 +60,15 @@ def test_update_email(self, mock_set_attribute): self.assertEqual(updated_user.email, updated_email) self.assertNotEqual(updated_user.email, initial_email) - mock_set_attribute.assert_called_with('update_email.username_mismatch', False) + mock_set_attribute.assert_any_call('update_email.username_mismatch', False) + mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False) + mock_set_attribute.assert_any_call('update_email.email_updated', 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): + 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(self.strategy, {'email': None, 'username': 'test_user'}, user=self.user) @@ -70,7 +76,9 @@ def test_update_email_with_none(self, mock_set_attribute): updated_user = User.objects.get(pk=self.user.pk) self.assertEqual(updated_user.email, old_email) - mock_set_attribute.assert_called_with('update_email.username_mismatch', False) + mock_set_attribute.assert_any_call('update_email.username_mismatch', False) + mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False) + # email_updated should not be called since email wasn't updated @patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled') @patch('auth_backends.pipeline.logger') @@ -99,9 +107,11 @@ def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mo ) 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) + # email_updated should not be called since email wasn't updated @patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled') @patch('auth_backends.pipeline.logger') @@ -122,6 +132,8 @@ def test_username_mismatch_with_update_toggle_disabled(self, mock_set_attribute, 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) + mock_set_attribute.assert_any_call('update_email.email_updated', True) From 9eb32460e43fb632e51fe874b57cf609048a23c2 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Tue, 17 Jun 2025 15:41:36 +0000 Subject: [PATCH 11/12] fix: verify email_updated is not set when email is not updated in update_email tests --- auth_backends/tests/test_pipeline.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 68f6d3fd..e3249f85 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -78,7 +78,9 @@ def test_update_email_with_none(self, mock_set_attribute, mock_toggle): mock_set_attribute.assert_any_call('update_email.username_mismatch', False) mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False) - # email_updated should not be called since email wasn't updated + # Verify email_updated was not set + for call_args in mock_set_attribute.call_args_list: + self.assertNotEqual(call_args[0][0], 'update_email.email_updated') @patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled') @patch('auth_backends.pipeline.logger') @@ -111,7 +113,9 @@ def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mo 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) - # email_updated should not be called since email wasn't updated + # Verify email_updated was not set + for call_args in mock_set_attribute.call_args_list: + self.assertNotEqual(call_args[0][0], 'update_email.email_updated') @patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled') @patch('auth_backends.pipeline.logger') From 573ebfb20574faef685cef2efc2960573cad2dad Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Wed, 18 Jun 2025 06:43:57 +0000 Subject: [PATCH 12/12] fix: improve assertions for email_updated attribute in update_email tests --- auth_backends/tests/test_pipeline.py | 37 ++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 8 deletions(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index e3249f85..060dcb30 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -62,7 +62,7 @@ def test_update_email(self, mock_set_attribute, mock_toggle): mock_set_attribute.assert_any_call('update_email.username_mismatch', False) mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False) - mock_set_attribute.assert_any_call('update_email.email_updated', True) + 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') @@ -78,9 +78,7 @@ def test_update_email_with_none(self, mock_set_attribute, mock_toggle): mock_set_attribute.assert_any_call('update_email.username_mismatch', False) mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False) - # Verify email_updated was not set - for call_args in mock_set_attribute.call_args_list: - self.assertNotEqual(call_args[0][0], 'update_email.email_updated') + 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') @@ -113,9 +111,7 @@ def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mo 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) - # Verify email_updated was not set - for call_args in mock_set_attribute.call_args_list: - self.assertNotEqual(call_args[0][0], 'update_email.email_updated') + 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') @@ -140,4 +136,29 @@ def test_username_mismatch_with_update_toggle_disabled(self, mock_set_attribute, 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) - mock_set_attribute.assert_any_call('update_email.email_updated', 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}" + )