From d2dbaf8413be1734c1315fc9c72cc97f0921c892 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Thu, 24 Jul 2025 12:43:34 +0000 Subject: [PATCH 01/21] feat: add DEBUG_GET_USER_IF_EXISTS toggle and enhance get_user_if_exists function with debugging --- auth_backends/pipeline.py | 128 ++++++++++++++++++- auth_backends/tests/test_pipeline.py | 176 +++++++++++++++++++++++++-- 2 files changed, 288 insertions(+), 16 deletions(-) diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index d92259e6..7d97a74c 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -22,25 +22,143 @@ # .. toggle_target_removal_date: 2025-08-18 SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH = SettingToggle("SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH", default=False) +# .. toggle_name: DEBUG_GET_USER_IF_EXISTS +# .. toggle_implementation: SettingToggle +# .. toggle_default: False +# .. toggle_description: Enables detailed debugging and monitoring for the get_user_if_exists pipeline function. +# When enabled (True), additional logging and custom attributes will be set to help debug +# user account conflicts and authentication issues. +# .. toggle_use_cases: temporary +# .. toggle_creation_date: 2025-07-23 +# .. toggle_target_removal_date: 2025-09-23 +DEBUG_GET_USER_IF_EXISTS = SettingToggle("DEBUG_GET_USER_IF_EXISTS", default=False) + # pylint: disable=unused-argument # The function parameters must be named exactly as they are below. # Do not change them to appease Pylint. def get_user_if_exists(strategy, details, user=None, *args, **kwargs): # pylint: disable=keyword-arg-before-vararg - """Return a User with the given username iff the User exists.""" + """Return a User with the given username iff the User exists. + + Enhanced with debugging capabilities to track user account conflicts and authentication issues. + """ + details_username = details.get('username') + + # Set custom attributes for debugging + # .. custom_attribute_name: get_user_if_exists.details_username + # .. custom_attribute_description: Records the username provided in the social details + # to help debug authentication and user lookup issues. + set_custom_attribute('get_user_if_exists.details_username', details_username) + + # .. custom_attribute_name: get_user_if_exists.user_provided + # .. custom_attribute_description: Indicates whether a user object was already provided + # to the pipeline function, which affects the lookup logic. + set_custom_attribute('get_user_if_exists.user_provided', user is not None) + + # .. custom_attribute_name: get_user_if_exists.debug_enabled + # .. custom_attribute_description: Tracks whether the DEBUG_GET_USER_IF_EXISTS + # toggle is enabled during this pipeline execution. + set_custom_attribute('get_user_if_exists.debug_enabled', DEBUG_GET_USER_IF_EXISTS.is_enabled()) + if user: + # User is already provided - this typically happens when user exists from previous pipeline steps + existing_username = getattr(user, 'username', None) + + # .. custom_attribute_name: get_user_if_exists.existing_user_username + # .. custom_attribute_description: Records the username of the existing user object + # when a user is already provided to the pipeline. + set_custom_attribute('get_user_if_exists.existing_user_username', existing_username) + + # Check for username mismatch between provided user and details + username_mismatch = details_username != existing_username + + # .. custom_attribute_name: get_user_if_exists.username_mismatch + # .. custom_attribute_description: Tracks whether there's a mismatch between + # the username in details and the existing user's username. + set_custom_attribute('get_user_if_exists.username_mismatch', username_mismatch) + + if DEBUG_GET_USER_IF_EXISTS.is_enabled() or username_mismatch: + logger.info( + "get_user_if_exists: User already provided. Username mismatch: %s. " + "Details username: %s, Existing user username: %s", + username_mismatch, + details_username, + existing_username + ) + + if username_mismatch: + logger.warning( + "Username mismatch in get_user_if_exists. Details username: %s, " + "Existing user username: %s. This may indicate an authentication issue.", + details_username, + existing_username + ) + return {'is_new': False} + + # No user provided, attempt to find user by username from details + if not details_username: + logger.warning("get_user_if_exists: No username provided in details") + # .. custom_attribute_name: get_user_if_exists.no_username_in_details + # .. custom_attribute_description: Indicates that no username was provided in the details, + # which may indicate an issue with the authentication provider. + set_custom_attribute('get_user_if_exists.no_username_in_details', True) + return {} + try: - username = details.get('username') + found_user = User.objects.get(username=details_username) + + # .. custom_attribute_name: get_user_if_exists.user_found + # .. custom_attribute_description: Indicates that a user was successfully found + # by username lookup in the database. + set_custom_attribute('get_user_if_exists.user_found', True) + + # .. custom_attribute_name: get_user_if_exists.found_user_id + # .. custom_attribute_description: Records the ID of the user found by username lookup + # to help track user account conflicts. + set_custom_attribute('get_user_if_exists.found_user_id', found_user.id) + + if DEBUG_GET_USER_IF_EXISTS.is_enabled(): + logger.info( + "get_user_if_exists: Found existing user with username '%s' (ID: %s)", + details_username, + found_user.id + ) # Return the user if it exists return { 'is_new': False, - 'user': User.objects.get(username=username) + 'user': found_user } except User.DoesNotExist: - # Fall to the default return value - pass + # .. custom_attribute_name: get_user_if_exists.user_found + # .. custom_attribute_description: Indicates that no user was found + # by username lookup in the database. + set_custom_attribute('get_user_if_exists.user_found', False) + + if DEBUG_GET_USER_IF_EXISTS.is_enabled(): + logger.info( + "get_user_if_exists: No user found with username '%s'", + details_username + ) + + except Exception as e: + # Handle any unexpected errors during user lookup + logger.error( + "get_user_if_exists: Unexpected error during user lookup for username '%s': %s", + details_username, + str(e) + ) + + # .. custom_attribute_name: get_user_if_exists.lookup_error + # .. custom_attribute_description: Indicates that an unexpected error occurred + # during user lookup, which may indicate database or system issues. + set_custom_attribute('get_user_if_exists.lookup_error', True) + + # .. custom_attribute_name: get_user_if_exists.error_message + # .. custom_attribute_description: Records the error message when an unexpected + # error occurs during user lookup. + set_custom_attribute('get_user_if_exists.error_message', str(e)) # Nothing to return since we don't have a user return {} diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 060dcb30..ff6c958c 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -18,23 +18,184 @@ def setUp(self): self.username = 'edx' self.details = {'username': self.username} - def test_no_user_exists(self): + @patch('auth_backends.pipeline.set_custom_attribute') + @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') + def test_no_user_exists(self, mock_debug_toggle, mock_set_attribute): """ Verify an empty dict is returned if no user exists. """ + mock_debug_toggle.return_value = False + actual = get_user_if_exists(None, self.details) self.assertDictEqual(actual, {}) - def test_existing_user(self): + # Verify custom attributes were set + mock_set_attribute.assert_any_call('get_user_if_exists.details_username', self.username) + mock_set_attribute.assert_any_call('get_user_if_exists.user_provided', False) + mock_set_attribute.assert_any_call('get_user_if_exists.debug_enabled', False) + mock_set_attribute.assert_any_call('get_user_if_exists.user_found', False) + + @patch('auth_backends.pipeline.set_custom_attribute') + @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') + def test_existing_user(self, mock_debug_toggle, mock_set_attribute): """ Verify a dict with the user and extra details is returned if the user exists. """ + mock_debug_toggle.return_value = False user = User.objects.create(username=self.username) + actual = get_user_if_exists(None, self.details) self.assertDictEqual(actual, {'is_new': False, 'user': user}) - def test_get_user_if_exists(self): + # Verify custom attributes were set + mock_set_attribute.assert_any_call('get_user_if_exists.details_username', self.username) + mock_set_attribute.assert_any_call('get_user_if_exists.user_provided', False) + mock_set_attribute.assert_any_call('get_user_if_exists.debug_enabled', False) + mock_set_attribute.assert_any_call('get_user_if_exists.user_found', True) + mock_set_attribute.assert_any_call('get_user_if_exists.found_user_id', user.id) + + @patch('auth_backends.pipeline.set_custom_attribute') + @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') + def test_get_user_if_exists_with_user_provided(self, mock_debug_toggle, mock_set_attribute): """ Verify only the details are returned if a user is passed to the function. """ + mock_debug_toggle.return_value = False user = User.objects.create(username=self.username) + actual = get_user_if_exists(None, self.details, user=user) self.assertDictEqual(actual, {'is_new': False}) + # Verify custom attributes were set + mock_set_attribute.assert_any_call('get_user_if_exists.details_username', self.username) + mock_set_attribute.assert_any_call('get_user_if_exists.user_provided', True) + mock_set_attribute.assert_any_call('get_user_if_exists.debug_enabled', False) + mock_set_attribute.assert_any_call('get_user_if_exists.existing_user_username', self.username) + mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', False) + + @patch('auth_backends.pipeline.logger') + @patch('auth_backends.pipeline.set_custom_attribute') + @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') + def test_username_mismatch_with_provided_user(self, mock_debug_toggle, mock_set_attribute, mock_logger): + """ Verify proper handling when there's a username mismatch with provided user. """ + mock_debug_toggle.return_value = False + user = User.objects.create(username='existing_user') + details = {'username': 'different_user'} + + actual = get_user_if_exists(None, details, user=user) + self.assertDictEqual(actual, {'is_new': False}) + + # Verify custom attributes were set + mock_set_attribute.assert_any_call('get_user_if_exists.details_username', 'different_user') + mock_set_attribute.assert_any_call('get_user_if_exists.user_provided', True) + mock_set_attribute.assert_any_call('get_user_if_exists.existing_user_username', 'existing_user') + mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', True) + + # Verify warning was logged + mock_logger.warning.assert_called_once_with( + "Username mismatch in get_user_if_exists. Details username: %s, " + "Existing user username: %s. This may indicate an authentication issue.", + 'different_user', + 'existing_user' + ) + + @patch('auth_backends.pipeline.logger') + @patch('auth_backends.pipeline.set_custom_attribute') + @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') + def test_debug_enabled_with_existing_user(self, mock_debug_toggle, mock_logger): + """ Verify debug logging when DEBUG_GET_USER_IF_EXISTS toggle is enabled. """ + mock_debug_toggle.return_value = True + user = User.objects.create(username=self.username) + + actual = get_user_if_exists(None, self.details) + self.assertDictEqual(actual, {'is_new': False, 'user': user}) + + # Verify debug logging + mock_logger.info.assert_called_with( + "get_user_if_exists: Found existing user with username '%s' (ID: %s)", + self.username, + user.id + ) + + @patch('auth_backends.pipeline.logger') + @patch('auth_backends.pipeline.set_custom_attribute') + @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') + def test_debug_enabled_no_user_found(self, mock_debug_toggle, mock_logger): + """ Verify debug logging when no user is found and debug is enabled. """ + mock_debug_toggle.return_value = True + + actual = get_user_if_exists(None, self.details) + self.assertDictEqual(actual, {}) + + # Verify debug logging + mock_logger.info.assert_called_with( + "get_user_if_exists: No user found with username '%s'", + self.username + ) + + @patch('auth_backends.pipeline.logger') + @patch('auth_backends.pipeline.set_custom_attribute') + @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') + def test_no_username_in_details(self, mock_debug_toggle, mock_set_attribute, mock_logger): + """ Verify proper handling when no username is provided in details. """ + mock_debug_toggle.return_value = False + details = {} # No username + + actual = get_user_if_exists(None, details) + self.assertDictEqual(actual, {}) + + # Verify custom attributes were set + mock_set_attribute.assert_any_call('get_user_if_exists.details_username', None) + mock_set_attribute.assert_any_call('get_user_if_exists.no_username_in_details', True) + + # Verify warning was logged + mock_logger.warning.assert_called_with("get_user_if_exists: No username provided in details") + + @patch('auth_backends.pipeline.User.objects.get') + @patch('auth_backends.pipeline.logger') + @patch('auth_backends.pipeline.set_custom_attribute') + @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') + def test_database_error_handling(self, mock_debug_toggle, mock_set_attribute, mock_logger, mock_user_get): + """ Verify proper handling of unexpected database errors. """ + mock_debug_toggle.return_value = False + mock_user_get.side_effect = Exception("Database connection error") + + actual = get_user_if_exists(None, self.details) + self.assertDictEqual(actual, {}) + + # Verify custom attributes were set + mock_set_attribute.assert_any_call('get_user_if_exists.lookup_error', True) + mock_set_attribute.assert_any_call('get_user_if_exists.error_message', "Database connection error") + + # Verify error was logged + mock_logger.error.assert_called_with( + "get_user_if_exists: Unexpected error during user lookup for username '%s': %s", + self.username, + "Database connection error" + ) + + @patch('auth_backends.pipeline.logger') + @patch('auth_backends.pipeline.set_custom_attribute') + @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') + def test_debug_enabled_with_username_mismatch(self, mock_debug_toggle, mock_logger): + """ Verify debug and warning logging when username mismatch occurs with debug enabled. """ + mock_debug_toggle.return_value = True + user = User.objects.create(username='existing_user') + details = {'username': 'different_user'} + + actual = get_user_if_exists(None, details, user=user) + self.assertDictEqual(actual, {'is_new': False}) + + # Verify both info and warning logs were called + mock_logger.info.assert_called_with( + "get_user_if_exists: User already provided. Username mismatch: %s. " + "Details username: %s, Existing user username: %s", + True, + 'different_user', + 'existing_user' + ) + + mock_logger.warning.assert_called_with( + "Username mismatch in get_user_if_exists. Details username: %s, " + "Existing user username: %s. This may indicate an authentication issue.", + 'different_user', + 'existing_user' + ) + class UpdateEmailPipelineTests(TestCase): """ Tests for the update_email pipeline function. """ @@ -139,14 +300,7 @@ def test_username_mismatch_with_update_toggle_disabled(self, mock_set_attribute, 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 - """ + """ Assert that a specific attribute was or was not set via set_custom_attribute. """ matching_calls = [ call for call in mock_set_attribute.call_args_list if call[0][0] == attribute_name From c3dd58f10bda44386727afba2ac8a76254b0ad40 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Thu, 24 Jul 2025 13:55:24 +0000 Subject: [PATCH 02/21] fix: remove redundant set_custom_attribute patch and unused mock_Set_attribute --- auth_backends/tests/test_pipeline.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index ff6c958c..a0bf6ce6 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -94,7 +94,6 @@ def test_username_mismatch_with_provided_user(self, mock_debug_toggle, mock_set_ ) @patch('auth_backends.pipeline.logger') - @patch('auth_backends.pipeline.set_custom_attribute') @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') def test_debug_enabled_with_existing_user(self, mock_debug_toggle, mock_logger): """ Verify debug logging when DEBUG_GET_USER_IF_EXISTS toggle is enabled. """ @@ -112,7 +111,6 @@ def test_debug_enabled_with_existing_user(self, mock_debug_toggle, mock_logger): ) @patch('auth_backends.pipeline.logger') - @patch('auth_backends.pipeline.set_custom_attribute') @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') def test_debug_enabled_no_user_found(self, mock_debug_toggle, mock_logger): """ Verify debug logging when no user is found and debug is enabled. """ @@ -169,7 +167,6 @@ def test_database_error_handling(self, mock_debug_toggle, mock_set_attribute, mo ) @patch('auth_backends.pipeline.logger') - @patch('auth_backends.pipeline.set_custom_attribute') @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') def test_debug_enabled_with_username_mismatch(self, mock_debug_toggle, mock_logger): """ Verify debug and warning logging when username mismatch occurs with debug enabled. """ From 3dca82ded4ce5934c1a814dfd2ebf990002f36d2 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 28 Jul 2025 07:35:57 +0000 Subject: [PATCH 03/21] feat: introduce IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle and updated changelog --- CHANGELOG.rst | 15 ++ auth_backends/__init__.py | 2 +- auth_backends/pipeline.py | 148 +++++-------------- auth_backends/tests/test_pipeline.py | 207 ++++++++++----------------- 4 files changed, 125 insertions(+), 247 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 741f59fa..ea9bb979 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,21 @@ Unreleased ---------- +[4.6.1] - 2025-07-25 +-------------------- + +Added +~~~~~ + +* Added IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle to handle username mismatches in authentication pipeline. +* Enhanced get_user_if_exists function with monitoring capabilities for debugging authentication conflicts. + +Fixed +~~~~~ + +* Fixed authentication issues where username mismatches between logged-in users and social auth details caused "user already exists" errors. +* Improved user account conflict resolution in devstack and stage environments. + [4.6.0] - 2025-06-18 -------------------- diff --git a/auth_backends/__init__.py b/auth_backends/__init__.py index aa7c8494..34383a18 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.6.0' # pragma: no cover +__version__ = '4.6.1' # pragma: no cover diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index 7d97a74c..560fa838 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -22,150 +22,68 @@ # .. toggle_target_removal_date: 2025-08-18 SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH = SettingToggle("SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH", default=False) -# .. toggle_name: DEBUG_GET_USER_IF_EXISTS +# .. toggle_name: IGNORE_LOGGED_IN_USER_ON_MISMATCH # .. toggle_implementation: SettingToggle -# .. toggle_default: False -# .. toggle_description: Enables detailed debugging and monitoring for the get_user_if_exists pipeline function. -# When enabled (True), additional logging and custom attributes will be set to help debug -# user account conflicts and authentication issues. +# .. toggle_default: True +# .. toggle_description: Controls behavior when there's a username mismatch between the logged-in user +# and social auth details. When enabled (True), ignores the logged-in user and proceeds with +# user lookup from social auth details. When disabled (False), proceeds with the logged-in user +# despite the mismatch. This toggle is for temporary rollout only to ensure we don't create bugs. # .. toggle_use_cases: temporary -# .. toggle_creation_date: 2025-07-23 -# .. toggle_target_removal_date: 2025-09-23 -DEBUG_GET_USER_IF_EXISTS = SettingToggle("DEBUG_GET_USER_IF_EXISTS", default=False) +# .. toggle_creation_date: 2025-07-25 +# .. toggle_target_removal_date: 2025-09-25 +IGNORE_LOGGED_IN_USER_ON_MISMATCH = SettingToggle("IGNORE_LOGGED_IN_USER_ON_MISMATCH", default=True) # pylint: disable=unused-argument # The function parameters must be named exactly as they are below. # Do not change them to appease Pylint. def get_user_if_exists(strategy, details, user=None, *args, **kwargs): # pylint: disable=keyword-arg-before-vararg - """Return a User with the given username iff the User exists. - - Enhanced with debugging capabilities to track user account conflicts and authentication issues. """ - details_username = details.get('username') - - # Set custom attributes for debugging - # .. custom_attribute_name: get_user_if_exists.details_username - # .. custom_attribute_description: Records the username provided in the social details - # to help debug authentication and user lookup issues. - set_custom_attribute('get_user_if_exists.details_username', details_username) - - # .. custom_attribute_name: get_user_if_exists.user_provided - # .. custom_attribute_description: Indicates whether a user object was already provided - # to the pipeline function, which affects the lookup logic. - set_custom_attribute('get_user_if_exists.user_provided', user is not None) - - # .. custom_attribute_name: get_user_if_exists.debug_enabled - # .. custom_attribute_description: Tracks whether the DEBUG_GET_USER_IF_EXISTS - # toggle is enabled during this pipeline execution. - set_custom_attribute('get_user_if_exists.debug_enabled', DEBUG_GET_USER_IF_EXISTS.is_enabled()) - + Return a User with the given username iff the User exists. + """ if user: - # User is already provided - this typically happens when user exists from previous pipeline steps - existing_username = getattr(user, 'username', None) - - # .. custom_attribute_name: get_user_if_exists.existing_user_username - # .. custom_attribute_description: Records the username of the existing user object - # when a user is already provided to the pipeline. - set_custom_attribute('get_user_if_exists.existing_user_username', existing_username) - - # Check for username mismatch between provided user and details - username_mismatch = details_username != existing_username + # Check for username mismatch and toggle behavior + details_username = details.get('username') + user_username = getattr(user, 'username', None) + username_mismatch = details_username != user_username # .. custom_attribute_name: get_user_if_exists.username_mismatch # .. custom_attribute_description: Tracks whether there's a mismatch between - # the username in details and the existing user's 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('get_user_if_exists.username_mismatch', username_mismatch) - if DEBUG_GET_USER_IF_EXISTS.is_enabled() or username_mismatch: - logger.info( - "get_user_if_exists: User already provided. Username mismatch: %s. " - "Details username: %s, Existing user username: %s", - username_mismatch, - details_username, - existing_username - ) + # .. custom_attribute_name: get_user_if_exists.ignore_toggle_enabled + # .. custom_attribute_description: Tracks whether the IGNORE_LOGGED_IN_USER_ON_MISMATCH + # toggle is enabled during this pipeline execution. + set_custom_attribute('get_user_if_exists.ignore_toggle_enabled', IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled()) - if username_mismatch: - logger.warning( - "Username mismatch in get_user_if_exists. Details username: %s, " - "Existing user username: %s. This may indicate an authentication issue.", - details_username, - existing_username + if username_mismatch and IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled(): + logger.info( + "Username mismatch detected. Details: %s, User: %s. Ignoring logged-in user.", + details_username, user_username ) - - return {'is_new': False} - - # No user provided, attempt to find user by username from details - if not details_username: - logger.warning("get_user_if_exists: No username provided in details") - # .. custom_attribute_name: get_user_if_exists.no_username_in_details - # .. custom_attribute_description: Indicates that no username was provided in the details, - # which may indicate an issue with the authentication provider. - set_custom_attribute('get_user_if_exists.no_username_in_details', True) - return {} + else: + return {'is_new': False} try: - found_user = User.objects.get(username=details_username) - - # .. custom_attribute_name: get_user_if_exists.user_found - # .. custom_attribute_description: Indicates that a user was successfully found - # by username lookup in the database. - set_custom_attribute('get_user_if_exists.user_found', True) - - # .. custom_attribute_name: get_user_if_exists.found_user_id - # .. custom_attribute_description: Records the ID of the user found by username lookup - # to help track user account conflicts. - set_custom_attribute('get_user_if_exists.found_user_id', found_user.id) - - if DEBUG_GET_USER_IF_EXISTS.is_enabled(): - logger.info( - "get_user_if_exists: Found existing user with username '%s' (ID: %s)", - details_username, - found_user.id - ) + username = details.get('username') - # Return the user if it exists return { 'is_new': False, - 'user': found_user + 'user': User.objects.get(username=username) } except User.DoesNotExist: - # .. custom_attribute_name: get_user_if_exists.user_found - # .. custom_attribute_description: Indicates that no user was found - # by username lookup in the database. - set_custom_attribute('get_user_if_exists.user_found', False) - - if DEBUG_GET_USER_IF_EXISTS.is_enabled(): - logger.info( - "get_user_if_exists: No user found with username '%s'", - details_username - ) + pass - except Exception as e: - # Handle any unexpected errors during user lookup - logger.error( - "get_user_if_exists: Unexpected error during user lookup for username '%s': %s", - details_username, - str(e) - ) - - # .. custom_attribute_name: get_user_if_exists.lookup_error - # .. custom_attribute_description: Indicates that an unexpected error occurred - # during user lookup, which may indicate database or system issues. - set_custom_attribute('get_user_if_exists.lookup_error', True) - - # .. custom_attribute_name: get_user_if_exists.error_message - # .. custom_attribute_description: Records the error message when an unexpected - # error occurs during user lookup. - set_custom_attribute('get_user_if_exists.error_message', str(e)) - - # Nothing to return since we don't have a user return {} 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.""" + """ + Update the user's email address using data from provider. + """ if user: # Get usernames for comparison, using defensive coding diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index a0bf6ce6..bfdbe70f 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -11,7 +11,9 @@ class GetUserIfExistsPipelineTests(TestCase): - """ Tests for the get_user_if_exists pipeline function. """ + """ + Tests for the get_user_if_exists pipeline function. + """ def setUp(self): super().setUp() @@ -19,183 +21,116 @@ def setUp(self): self.details = {'username': self.username} @patch('auth_backends.pipeline.set_custom_attribute') - @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') - def test_no_user_exists(self, mock_debug_toggle, mock_set_attribute): + @patch('auth_backends.pipeline.IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled') + def test_no_user_exists(self, mock_toggle, mock_set_attribute): """ Verify an empty dict is returned if no user exists. """ - mock_debug_toggle.return_value = False + mock_toggle.return_value = True actual = get_user_if_exists(None, self.details) self.assertDictEqual(actual, {}) - # Verify custom attributes were set - mock_set_attribute.assert_any_call('get_user_if_exists.details_username', self.username) - mock_set_attribute.assert_any_call('get_user_if_exists.user_provided', False) - mock_set_attribute.assert_any_call('get_user_if_exists.debug_enabled', False) - mock_set_attribute.assert_any_call('get_user_if_exists.user_found', False) + mock_set_attribute.assert_not_called() @patch('auth_backends.pipeline.set_custom_attribute') - @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') - def test_existing_user(self, mock_debug_toggle, mock_set_attribute): - """ Verify a dict with the user and extra details is returned if the user exists. """ - mock_debug_toggle.return_value = False + @patch('auth_backends.pipeline.IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled') + def test_existing_user(self, mock_toggle, mock_set_attribute): + """ + Verify a dict with the user and extra details is returned if the user exists. + """ + mock_toggle.return_value = True user = User.objects.create(username=self.username) actual = get_user_if_exists(None, self.details) self.assertDictEqual(actual, {'is_new': False, 'user': user}) - # Verify custom attributes were set - mock_set_attribute.assert_any_call('get_user_if_exists.details_username', self.username) - mock_set_attribute.assert_any_call('get_user_if_exists.user_provided', False) - mock_set_attribute.assert_any_call('get_user_if_exists.debug_enabled', False) - mock_set_attribute.assert_any_call('get_user_if_exists.user_found', True) - mock_set_attribute.assert_any_call('get_user_if_exists.found_user_id', user.id) + mock_set_attribute.assert_not_called() @patch('auth_backends.pipeline.set_custom_attribute') - @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') - def test_get_user_if_exists_with_user_provided(self, mock_debug_toggle, mock_set_attribute): - """ Verify only the details are returned if a user is passed to the function. """ - mock_debug_toggle.return_value = False + @patch('auth_backends.pipeline.IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled') + def test_get_user_if_exists_with_user_provided_no_mismatch(self, mock_toggle, mock_set_attribute): + """ + Verify only the details are returned if a user is passed with no username mismatch. + """ + mock_toggle.return_value = True user = User.objects.create(username=self.username) actual = get_user_if_exists(None, self.details, user=user) self.assertDictEqual(actual, {'is_new': False}) - # Verify custom attributes were set - mock_set_attribute.assert_any_call('get_user_if_exists.details_username', self.username) - mock_set_attribute.assert_any_call('get_user_if_exists.user_provided', True) - mock_set_attribute.assert_any_call('get_user_if_exists.debug_enabled', False) - mock_set_attribute.assert_any_call('get_user_if_exists.existing_user_username', self.username) mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', False) + mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', True) @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') - @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') - def test_username_mismatch_with_provided_user(self, mock_debug_toggle, mock_set_attribute, mock_logger): - """ Verify proper handling when there's a username mismatch with provided user. """ - mock_debug_toggle.return_value = False + @patch('auth_backends.pipeline.IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled') + def test_username_mismatch_toggle_enabled(self, mock_toggle, mock_set_attribute, mock_logger): + """ + Verify user lookup when username mismatch occurs and toggle is enabled. + """ + mock_toggle.return_value = True user = User.objects.create(username='existing_user') + target_user = User.objects.create(username='different_user') details = {'username': 'different_user'} actual = get_user_if_exists(None, details, user=user) - self.assertDictEqual(actual, {'is_new': False}) + self.assertDictEqual(actual, {'is_new': False, 'user': target_user}) - # Verify custom attributes were set - mock_set_attribute.assert_any_call('get_user_if_exists.details_username', 'different_user') - mock_set_attribute.assert_any_call('get_user_if_exists.user_provided', True) - mock_set_attribute.assert_any_call('get_user_if_exists.existing_user_username', 'existing_user') mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', True) + mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', True) - # Verify warning was logged - mock_logger.warning.assert_called_once_with( - "Username mismatch in get_user_if_exists. Details username: %s, " - "Existing user username: %s. This may indicate an authentication issue.", + mock_logger.info.assert_called_once_with( + "Username mismatch detected. Details: %s, User: %s. Ignoring logged-in user.", 'different_user', 'existing_user' ) - @patch('auth_backends.pipeline.logger') - @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') - def test_debug_enabled_with_existing_user(self, mock_debug_toggle, mock_logger): - """ Verify debug logging when DEBUG_GET_USER_IF_EXISTS toggle is enabled. """ - mock_debug_toggle.return_value = True - user = User.objects.create(username=self.username) - - actual = get_user_if_exists(None, self.details) - self.assertDictEqual(actual, {'is_new': False, 'user': user}) - - # Verify debug logging - mock_logger.info.assert_called_with( - "get_user_if_exists: Found existing user with username '%s' (ID: %s)", - self.username, - user.id - ) - - @patch('auth_backends.pipeline.logger') - @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') - def test_debug_enabled_no_user_found(self, mock_debug_toggle, mock_logger): - """ Verify debug logging when no user is found and debug is enabled. """ - mock_debug_toggle.return_value = True - - actual = get_user_if_exists(None, self.details) - self.assertDictEqual(actual, {}) - - # Verify debug logging - mock_logger.info.assert_called_with( - "get_user_if_exists: No user found with username '%s'", - self.username - ) - @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') - @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') - def test_no_username_in_details(self, mock_debug_toggle, mock_set_attribute, mock_logger): - """ Verify proper handling when no username is provided in details. """ - mock_debug_toggle.return_value = False - details = {} # No username + @patch('auth_backends.pipeline.IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled') + def test_username_mismatch_toggle_disabled(self, mock_toggle, mock_set_attribute, mock_logger): + """ + Verify logged-in user is returned when username mismatch occurs and toggle is disabled. + """ + mock_toggle.return_value = False + user = User.objects.create(username='existing_user') + details = {'username': 'different_user'} - actual = get_user_if_exists(None, details) - self.assertDictEqual(actual, {}) + actual = get_user_if_exists(None, details, user=user) + self.assertDictEqual(actual, {'is_new': False}) - # Verify custom attributes were set - mock_set_attribute.assert_any_call('get_user_if_exists.details_username', None) - mock_set_attribute.assert_any_call('get_user_if_exists.no_username_in_details', True) + mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', True) + mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', False) - # Verify warning was logged - mock_logger.warning.assert_called_with("get_user_if_exists: No username provided in details") + mock_logger.info.assert_not_called() - @patch('auth_backends.pipeline.User.objects.get') @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') - @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') - def test_database_error_handling(self, mock_debug_toggle, mock_set_attribute, mock_logger, mock_user_get): - """ Verify proper handling of unexpected database errors. """ - mock_debug_toggle.return_value = False - mock_user_get.side_effect = Exception("Database connection error") - - actual = get_user_if_exists(None, self.details) - self.assertDictEqual(actual, {}) - - # Verify custom attributes were set - mock_set_attribute.assert_any_call('get_user_if_exists.lookup_error', True) - mock_set_attribute.assert_any_call('get_user_if_exists.error_message', "Database connection error") - - # Verify error was logged - mock_logger.error.assert_called_with( - "get_user_if_exists: Unexpected error during user lookup for username '%s': %s", - self.username, - "Database connection error" - ) - - @patch('auth_backends.pipeline.logger') - @patch('auth_backends.pipeline.DEBUG_GET_USER_IF_EXISTS.is_enabled') - def test_debug_enabled_with_username_mismatch(self, mock_debug_toggle, mock_logger): - """ Verify debug and warning logging when username mismatch occurs with debug enabled. """ - mock_debug_toggle.return_value = True + @patch('auth_backends.pipeline.IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled') + def test_username_mismatch_target_user_not_found(self, mock_toggle, mock_set_attribute, mock_logger): + """ + Verify empty dict when username mismatch occurs but target user doesn't exist. + """ + mock_toggle.return_value = True user = User.objects.create(username='existing_user') - details = {'username': 'different_user'} + details = {'username': 'nonexistent_user'} actual = get_user_if_exists(None, details, user=user) - self.assertDictEqual(actual, {'is_new': False}) + self.assertDictEqual(actual, {}) - # Verify both info and warning logs were called - mock_logger.info.assert_called_with( - "get_user_if_exists: User already provided. Username mismatch: %s. " - "Details username: %s, Existing user username: %s", - True, - 'different_user', - 'existing_user' - ) + mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', True) + mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', True) - mock_logger.warning.assert_called_with( - "Username mismatch in get_user_if_exists. Details username: %s, " - "Existing user username: %s. This may indicate an authentication issue.", - 'different_user', + mock_logger.info.assert_called_once_with( + "Username mismatch detected. Details: %s, User: %s. Ignoring logged-in user.", + 'nonexistent_user', 'existing_user' ) class UpdateEmailPipelineTests(TestCase): - """ Tests for the update_email pipeline function. """ + """ + Tests for the update_email pipeline function. + """ def setUp(self): super().setUp() @@ -205,7 +140,9 @@ def setUp(self): @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. """ + """ + 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) @@ -225,7 +162,9 @@ def test_update_email(self, mock_set_attribute, mock_toggle): @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. """ + """ + Verify that user email is not updated if email value is None. + """ mock_toggle.return_value = False old_email = self.user.email @@ -242,7 +181,9 @@ def test_update_email_with_none(self, mock_set_attribute, mock_toggle): @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. """ + """ + 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 @@ -275,7 +216,9 @@ def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mo @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. """ + """ + Verify that email is updated when usernames don't match but toggle is disabled. + """ mock_toggle.return_value = False old_email = self.user.email @@ -297,7 +240,9 @@ def test_username_mismatch_with_update_toggle_disabled(self, mock_set_attribute, 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. """ + """ + Assert that a specific attribute was or was not set via set_custom_attribute. + """ matching_calls = [ call for call in mock_set_attribute.call_args_list if call[0][0] == attribute_name From 861508434d18bd553f9f51173b0e536aab8166f4 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 28 Jul 2025 12:21:00 +0000 Subject: [PATCH 04/21] fix: improve logging message for username mismatch in get_user_if_exists function --- auth_backends/pipeline.py | 2 +- auth_backends/tests/test_pipeline.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index 560fa838..e7afd0cb 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -61,7 +61,7 @@ def get_user_if_exists(strategy, details, user=None, *args, **kwargs): # pylint if username_mismatch and IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled(): logger.info( - "Username mismatch detected. Details: %s, User: %s. Ignoring logged-in user.", + "Username mismatch detected. Username from Details: %s, Username from User: %s.", details_username, user_username ) else: diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index bfdbe70f..67db865e 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -23,7 +23,9 @@ def setUp(self): @patch('auth_backends.pipeline.set_custom_attribute') @patch('auth_backends.pipeline.IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled') def test_no_user_exists(self, mock_toggle, mock_set_attribute): - """ Verify an empty dict is returned if no user exists. """ + """ + Verify an empty dict is returned if no user exists. + """ mock_toggle.return_value = True actual = get_user_if_exists(None, self.details) From 3bc63ef65ec024947e024e59e3d363171da5379a Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 28 Jul 2025 12:26:54 +0000 Subject: [PATCH 05/21] fix: update logging message for username mismatch in get_user_if_exists tests --- auth_backends/tests/test_pipeline.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 67db865e..c3a1d89e 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -81,7 +81,7 @@ def test_username_mismatch_toggle_enabled(self, mock_toggle, mock_set_attribute, mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', True) mock_logger.info.assert_called_once_with( - "Username mismatch detected. Details: %s, User: %s. Ignoring logged-in user.", + "Username mismatch detected. Username from Details: %s, Username from User: %s.", 'different_user', 'existing_user' ) @@ -123,7 +123,7 @@ def test_username_mismatch_target_user_not_found(self, mock_toggle, mock_set_att mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', True) mock_logger.info.assert_called_once_with( - "Username mismatch detected. Details: %s, User: %s. Ignoring logged-in user.", + "Username mismatch detected. Username from Details: %s, Username from User: %s.", 'nonexistent_user', 'existing_user' ) From 19e06a96bd0aed477a824a4bd36f235e689c6c59 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Wed, 30 Jul 2025 11:36:38 +0530 Subject: [PATCH 06/21] docs: Update CHANGELOG.rst Co-authored-by: Robert Raposa --- CHANGELOG.rst | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ea9bb979..1e09a9d2 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -16,17 +16,13 @@ Unreleased [4.6.1] - 2025-07-25 -------------------- -Added -~~~~~ - -* Added IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle to handle username mismatches in authentication pipeline. -* Enhanced get_user_if_exists function with monitoring capabilities for debugging authentication conflicts. - Fixed ~~~~~ * Fixed authentication issues where username mismatches between logged-in users and social auth details caused "user already exists" errors. -* Improved user account conflict resolution in devstack and stage environments. + + * Added temporary rollout toggle IGNORE_LOGGED_IN_USER_ON_MISMATCH to ensure the bug fix doesn't introduce any issues.. + * Enhanced get_user_if_exists function with monitoring capabilities for debugging authentication conflicts. [4.6.0] - 2025-06-18 -------------------- From fb5db54fe7143207e636c28764a54f50d91c04ae Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Thu, 31 Jul 2025 05:34:34 +0000 Subject: [PATCH 07/21] feat: enhance get_user_if_exists function with toggle tracking and refactor tests for improved --- auth_backends/pipeline.py | 10 +- auth_backends/tests/test_pipeline.py | 185 ++++++++++++--------------- 2 files changed, 84 insertions(+), 111 deletions(-) diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index e7afd0cb..0a723be1 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -42,6 +42,11 @@ def get_user_if_exists(strategy, details, user=None, *args, **kwargs): # pylint """ Return a User with the given username iff the User exists. """ + # .. custom_attribute_name: get_user_if_exists.ignore_toggle_enabled + # .. custom_attribute_description: Tracks whether the IGNORE_LOGGED_IN_USER_ON_MISMATCH + # toggle is enabled during this pipeline execution. + set_custom_attribute('get_user_if_exists.ignore_toggle_enabled', IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled()) + if user: # Check for username mismatch and toggle behavior details_username = details.get('username') @@ -54,11 +59,6 @@ def get_user_if_exists(strategy, details, user=None, *args, **kwargs): # pylint # True if usernames don't match, False if they match. set_custom_attribute('get_user_if_exists.username_mismatch', username_mismatch) - # .. custom_attribute_name: get_user_if_exists.ignore_toggle_enabled - # .. custom_attribute_description: Tracks whether the IGNORE_LOGGED_IN_USER_ON_MISMATCH - # toggle is enabled during this pipeline execution. - set_custom_attribute('get_user_if_exists.ignore_toggle_enabled', IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled()) - if username_mismatch and IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled(): logger.info( "Username mismatch detected. Username from Details: %s, Username from User: %s.", diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index c3a1d89e..e19abef9 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 +import ddt from django.contrib.auth import get_user_model -from django.test import TestCase +from django.test import TestCase, override_settings from social_django.utils import load_strategy from auth_backends.pipeline import get_user_if_exists, update_email @@ -10,6 +11,7 @@ User = get_user_model() +@ddt.ddt class GetUserIfExistsPipelineTests(TestCase): """ Tests for the get_user_if_exists pipeline function. @@ -20,113 +22,84 @@ def setUp(self): self.username = 'edx' self.details = {'username': self.username} - @patch('auth_backends.pipeline.set_custom_attribute') - @patch('auth_backends.pipeline.IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled') - def test_no_user_exists(self, mock_toggle, mock_set_attribute): - """ - Verify an empty dict is returned if no user exists. - """ - mock_toggle.return_value = True - - actual = get_user_if_exists(None, self.details) - self.assertDictEqual(actual, {}) - - mock_set_attribute.assert_not_called() - - @patch('auth_backends.pipeline.set_custom_attribute') - @patch('auth_backends.pipeline.IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled') - def test_existing_user(self, mock_toggle, mock_set_attribute): - """ - Verify a dict with the user and extra details is returned if the user exists. - """ - mock_toggle.return_value = True - user = User.objects.create(username=self.username) - - actual = get_user_if_exists(None, self.details) - self.assertDictEqual(actual, {'is_new': False, 'user': user}) - - mock_set_attribute.assert_not_called() - - @patch('auth_backends.pipeline.set_custom_attribute') - @patch('auth_backends.pipeline.IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled') - def test_get_user_if_exists_with_user_provided_no_mismatch(self, mock_toggle, mock_set_attribute): - """ - Verify only the details are returned if a user is passed with no username mismatch. - """ - mock_toggle.return_value = True - user = User.objects.create(username=self.username) - - actual = get_user_if_exists(None, self.details, user=user) - self.assertDictEqual(actual, {'is_new': False}) - - mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', False) - mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', True) - + @ddt.data(True, False) + def test_no_user_exists(self, setting_value): + """ + Verify an empty dict is returned if no user exists regardless of setting. + """ + with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=setting_value): + actual = get_user_if_exists(None, self.details) + self.assertDictEqual(actual, {}) + + @ddt.data( + # (test_config, expected_result) + ({'setting': True, 'user_provided': False, 'username_match': True, 'target_exists': True}, + {'is_new': False, 'user': 'found_user'}), + ({'setting': False, 'user_provided': False, 'username_match': True, 'target_exists': True}, + {'is_new': False, 'user': 'found_user'}), + ({'setting': True, 'user_provided': True, 'username_match': True, 'target_exists': True}, + {'is_new': False}), + ({'setting': False, 'user_provided': True, 'username_match': True, 'target_exists': True}, + {'is_new': False}), + ({'setting': True, 'user_provided': True, 'username_match': False, 'target_exists': True}, + {'is_new': False, 'user': 'target_user'}), + ({'setting': False, 'user_provided': True, 'username_match': False, 'target_exists': True}, + {'is_new': False}), + ({'setting': True, 'user_provided': True, 'username_match': False, 'target_exists': False}, + {}), + ) + @ddt.unpack @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') - @patch('auth_backends.pipeline.IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled') - def test_username_mismatch_toggle_enabled(self, mock_toggle, mock_set_attribute, mock_logger): - """ - Verify user lookup when username mismatch occurs and toggle is enabled. - """ - mock_toggle.return_value = True - user = User.objects.create(username='existing_user') - target_user = User.objects.create(username='different_user') - details = {'username': 'different_user'} - - actual = get_user_if_exists(None, details, user=user) - self.assertDictEqual(actual, {'is_new': False, 'user': target_user}) - - mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', True) - mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', True) - - mock_logger.info.assert_called_once_with( - "Username mismatch detected. Username from Details: %s, Username from User: %s.", - 'different_user', - 'existing_user' - ) - - @patch('auth_backends.pipeline.logger') - @patch('auth_backends.pipeline.set_custom_attribute') - @patch('auth_backends.pipeline.IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled') - def test_username_mismatch_toggle_disabled(self, mock_toggle, mock_set_attribute, mock_logger): - """ - Verify logged-in user is returned when username mismatch occurs and toggle is disabled. - """ - mock_toggle.return_value = False - user = User.objects.create(username='existing_user') - details = {'username': 'different_user'} - - actual = get_user_if_exists(None, details, user=user) - self.assertDictEqual(actual, {'is_new': False}) - - mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', True) - mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', False) - - mock_logger.info.assert_not_called() - - @patch('auth_backends.pipeline.logger') - @patch('auth_backends.pipeline.set_custom_attribute') - @patch('auth_backends.pipeline.IGNORE_LOGGED_IN_USER_ON_MISMATCH.is_enabled') - def test_username_mismatch_target_user_not_found(self, mock_toggle, mock_set_attribute, mock_logger): - """ - Verify empty dict when username mismatch occurs but target user doesn't exist. - """ - mock_toggle.return_value = True - user = User.objects.create(username='existing_user') - details = {'username': 'nonexistent_user'} - - actual = get_user_if_exists(None, details, user=user) - self.assertDictEqual(actual, {}) - - mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', True) - mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', True) - - mock_logger.info.assert_called_once_with( - "Username mismatch detected. Username from Details: %s, Username from User: %s.", - 'nonexistent_user', - 'existing_user' - ) + def test_user_scenarios(self, test_config, expected_result, mock_set_attribute, mock_logger): + """ + Test various user scenarios with different settings and user conditions. + """ + setting_value = test_config['setting'] + user_provided = test_config['user_provided'] + username_match = test_config['username_match'] + target_exists = test_config['target_exists'] + + with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=setting_value): + if user_provided: + user = User.objects.create(username='existing_user') + if username_match: + details = {'username': 'existing_user'} + else: + if target_exists: + target_user = User.objects.create(username='different_user') + details = {'username': 'different_user'} + + if expected_result.get('user') == 'target_user': + expected_result['user'] = target_user + else: + details = {'username': 'nonexistent_user'} + else: + user = None + found_user = User.objects.create(username=self.username) + details = self.details + + if expected_result.get('user') == 'found_user': + expected_result['user'] = found_user + + actual = get_user_if_exists(None, details, user=user) + self.assertDictEqual(actual, expected_result) + + mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', setting_value) + + if user_provided: + mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', not username_match) + + if user_provided and not username_match and setting_value: + mock_logger.info.assert_called_once() + if not target_exists: + mock_logger.info.assert_called_with( + "Username mismatch detected. Username from Details: %s, Username from User: %s.", + 'nonexistent_user', + 'existing_user' + ) + else: + mock_logger.info.assert_not_called() class UpdateEmailPipelineTests(TestCase): From b4664820fc23a9f1591a92d5be550b3c984254e9 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Thu, 31 Jul 2025 07:49:50 +0000 Subject: [PATCH 08/21] fix: added ddt module inrequirements/test.in --- requirements/test.in | 1 + requirements/test.txt | 112 +++++++++++++++++++++++++++++++----------- 2 files changed, 83 insertions(+), 30 deletions(-) diff --git a/requirements/test.in b/requirements/test.in index 210ca42b..20e2a976 100644 --- a/requirements/test.in +++ b/requirements/test.in @@ -4,6 +4,7 @@ -r base.txt # Core dependencies coverage +ddt # for running multiple test cases with multiple input edx-lint httpretty pycodestyle diff --git a/requirements/test.txt b/requirements/test.txt index 6add5676..1423e973 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -6,15 +6,15 @@ # argparse==1.4.0 # via unittest2 -asgiref==3.8.1 +asgiref==3.9.1 # via # -r requirements/base.txt # django -astroid==3.3.9 +astroid==3.3.11 # via # pylint # pylint-celery -certifi==2025.1.31 +certifi==2025.7.14 # via # -r requirements/base.txt # requests @@ -22,28 +22,36 @@ cffi==1.17.1 # via # -r requirements/base.txt # cryptography -charset-normalizer==3.4.1 + # pynacl +charset-normalizer==3.4.2 # via # -r requirements/base.txt # requests -click==8.1.8 +click==8.2.1 # via + # -r requirements/base.txt # click-log # code-annotations + # edx-django-utils # edx-lint click-log==0.4.0 # via edx-lint code-annotations==2.3.0 - # via edx-lint -coverage[toml]==7.8.0 + # via + # -r requirements/base.txt + # edx-lint + # edx-toggles +coverage[toml]==7.10.1 # via # -r requirements/test.in # pytest-cov -cryptography==44.0.2 +cryptography==45.0.5 # via # -r requirements/base.txt # pyjwt # social-auth-core +ddt==1.7.2 + # via -r requirements/test.in defusedxml==0.7.1 # via # -r requirements/base.txt @@ -51,17 +59,37 @@ defusedxml==0.7.1 # social-auth-core dill==0.4.0 # via pylint -distlib==0.3.9 +distlib==0.4.0 # via virtualenv # via # -c requirements/common_constraints.txt # -r requirements/base.txt + # django-crum + # django-waffle # edx-django-release-util + # edx-django-utils + # edx-toggles # social-auth-app-django +django-crum==0.7.9 + # via + # -r requirements/base.txt + # edx-django-utils + # edx-toggles +django-waffle==5.0.0 + # via + # -r requirements/base.txt + # edx-django-utils + # edx-toggles edx-django-release-util==1.5.0 # via -r requirements/test.in +edx-django-utils==8.0.0 + # via + # -r requirements/base.txt + # edx-toggles edx-lint==5.6.0 # via -r requirements/test.in +edx-toggles==5.4.1 + # via -r requirements/base.txt filelock==3.18.0 # via # tox @@ -77,14 +105,18 @@ iniconfig==2.1.0 isort==6.0.1 # via pylint jinja2==3.1.6 - # via code-annotations + # via + # -r requirements/base.txt + # code-annotations linecache2==1.0.0 # via traceback2 markupsafe==3.0.2 - # via jinja2 + # via + # -r requirements/base.txt + # jinja2 mccabe==0.7.0 # via pylint -oauthlib==3.2.2 +oauthlib==3.3.1 # via # -r requirements/base.txt # requests-oauthlib @@ -94,30 +126,39 @@ packaging==25.0 # pytest # tox pbr==6.1.1 - # via stevedore -platformdirs==4.3.7 + # via + # -r requirements/base.txt + # stevedore +platformdirs==4.3.8 # via # pylint # virtualenv -pluggy==1.5.0 +pluggy==1.6.0 # via # pytest + # pytest-cov # tox +psutil==7.0.0 + # via + # -r requirements/base.txt + # edx-django-utils py==1.11.0 # via tox -pycodestyle==2.13.0 +pycodestyle==2.14.0 # via -r requirements/test.in pycparser==2.22 # via # -r requirements/base.txt # cffi -pycryptodomex==3.22.0 +pycryptodomex==3.23.0 # via -r requirements/test.in +pygments==2.19.2 + # via pytest pyjwt[crypto]==2.10.1 # via # -r requirements/base.txt # social-auth-core -pylint==3.3.6 +pylint==3.3.7 # via # edx-lint # pylint-celery @@ -127,29 +168,36 @@ pylint-celery==0.3 # via edx-lint pylint-django==2.6.1 # via edx-lint -pylint-plugin-utils==0.8.2 +pylint-plugin-utils==0.9.0 # via # pylint-celery # pylint-django -pytest==8.3.5 +pynacl==1.5.0 + # via + # -r requirements/base.txt + # edx-django-utils +pytest==8.4.1 # via # pytest-cov # pytest-django -pytest-cov==6.1.1 +pytest-cov==6.2.1 # via -r requirements/test.in pytest-django==4.11.1 # via -r requirements/test.in python-slugify==8.0.4 - # via code-annotations + # via + # -r requirements/base.txt + # code-annotations python3-openid==3.2.0 # via # -r requirements/base.txt # social-auth-core pyyaml==6.0.2 # via + # -r requirements/base.txt # code-annotations # edx-django-release-util -requests==2.32.3 +requests==2.32.4 # via # -r requirements/base.txt # requests-oauthlib @@ -167,7 +215,7 @@ six==1.17.0 # unittest2 social-auth-app-django==5.4.3 # via -r requirements/base.txt -social-auth-core==4.5.6 +social-auth-core==4.7.0 # via # -r requirements/base.txt # social-auth-app-django @@ -176,10 +224,15 @@ sqlparse==0.5.3 # -r requirements/base.txt # django stevedore==5.4.1 - # via code-annotations + # via + # -r requirements/base.txt + # code-annotations + # edx-django-utils text-unidecode==1.3 - # via python-slugify -tomlkit==0.13.2 + # via + # -r requirements/base.txt + # python-slugify +tomlkit==0.13.3 # via pylint tox==3.28.0 # via @@ -189,12 +242,11 @@ traceback2==1.4.0 # via unittest2 unittest2==1.1.0 # via -r requirements/test.in -urllib3==2.2.3 +urllib3==2.5.0 # via - # -c requirements/common_constraints.txt # -r requirements/base.txt # requests -virtualenv==20.30.0 +virtualenv==20.32.0 # via tox # The following packages are considered to be unsafe in a requirements file: From 83a11d98c7b336d25eb26a21cae8197b5f842d71 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Thu, 31 Jul 2025 07:59:22 +0000 Subject: [PATCH 09/21] fix: added responses module in test.in --- requirements/test.in | 1 + requirements/test.txt | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/requirements/test.in b/requirements/test.in index 20e2a976..d72ac18c 100644 --- a/requirements/test.in +++ b/requirements/test.in @@ -11,6 +11,7 @@ pycodestyle pycryptodomex # used for crypto tests pytest-cov pytest-django +responses tox unittest2 edx-django-release-util # Contains the reserved keyword check diff --git a/requirements/test.txt b/requirements/test.txt index 1423e973..1ef86225 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -197,15 +197,19 @@ pyyaml==6.0.2 # -r requirements/base.txt # code-annotations # edx-django-release-util + # responses requests==2.32.4 # via # -r requirements/base.txt # requests-oauthlib + # responses # social-auth-core requests-oauthlib==2.0.0 # via # -r requirements/base.txt # social-auth-core +responses==0.25.7 + # via -r requirements/test.in six==1.17.0 # via # -r requirements/base.txt @@ -246,6 +250,7 @@ urllib3==2.5.0 # via # -r requirements/base.txt # requests + # responses virtualenv==20.32.0 # via tox From fc852847ef5d0b33d210c6dba0ef7fb59f683f87 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Thu, 31 Jul 2025 11:14:58 +0000 Subject: [PATCH 10/21] feat: added relevant comments in test and package fix --- auth_backends/tests/test_pipeline.py | 14 +++++++------- requirements/test.in | 1 + requirements/test.txt | 2 ++ 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index e19abef9..1cead18f 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -34,19 +34,19 @@ def test_no_user_exists(self, setting_value): @ddt.data( # (test_config, expected_result) ({'setting': True, 'user_provided': False, 'username_match': True, 'target_exists': True}, - {'is_new': False, 'user': 'found_user'}), + {'is_new': False, 'user': 'found_user'}), # setting=True, no current user ({'setting': False, 'user_provided': False, 'username_match': True, 'target_exists': True}, - {'is_new': False, 'user': 'found_user'}), + {'is_new': False, 'user': 'found_user'}), # setting=False, no current user ({'setting': True, 'user_provided': True, 'username_match': True, 'target_exists': True}, - {'is_new': False}), + {'is_new': False}), # setting=True, usernames match ({'setting': False, 'user_provided': True, 'username_match': True, 'target_exists': True}, - {'is_new': False}), + {'is_new': False}), # setting=False, usernames match ({'setting': True, 'user_provided': True, 'username_match': False, 'target_exists': True}, - {'is_new': False, 'user': 'target_user'}), + {'is_new': False, 'user': 'target_user'}), # setting=True, mismatch with target ({'setting': False, 'user_provided': True, 'username_match': False, 'target_exists': True}, - {'is_new': False}), + {'is_new': False}), # setting=False, mismatch with target ({'setting': True, 'user_provided': True, 'username_match': False, 'target_exists': False}, - {}), + {}), # setting=True, mismatch no target ) @ddt.unpack @patch('auth_backends.pipeline.logger') diff --git a/requirements/test.in b/requirements/test.in index d72ac18c..198fe455 100644 --- a/requirements/test.in +++ b/requirements/test.in @@ -13,5 +13,6 @@ pytest-cov pytest-django responses tox +typing_extensions unittest2 edx-django-release-util # Contains the reserved keyword check diff --git a/requirements/test.txt b/requirements/test.txt index 1ef86225..ee46bd39 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -244,6 +244,8 @@ tox==3.28.0 # -r requirements/test.in traceback2==1.4.0 # via unittest2 +typing-extensions==4.14.1 + # via -r requirements/test.in unittest2==1.1.0 # via -r requirements/test.in urllib3==2.5.0 From 65c30a617beb5cb5af57ed24fae9c9d9e67dc189 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 4 Aug 2025 13:59:08 +0000 Subject: [PATCH 11/21] fix: updated use of response and typing_extension with social-core version upgrade --- auth_backends/backends.py | 1 - auth_backends/tests/test_backends.py | 19 ++++++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/auth_backends/backends.py b/auth_backends/backends.py index f3fc44c0..453bffaf 100644 --- a/auth_backends/backends.py +++ b/auth_backends/backends.py @@ -31,7 +31,6 @@ def _to_language(locale): return locale.replace('_', '-').lower() -# pylint: disable=abstract-method class EdXOAuth2(BaseOAuth2): """ IMPORTANT: The oauth2 application must have access to the ``user_id`` scope in order diff --git a/auth_backends/tests/test_backends.py b/auth_backends/tests/test_backends.py index 2c94cf16..055cf690 100644 --- a/auth_backends/tests/test_backends.py +++ b/auth_backends/tests/test_backends.py @@ -4,6 +4,7 @@ from calendar import timegm import jwt +import responses import six from Cryptodome.PublicKey import RSA from django.core.cache import cache @@ -37,10 +38,13 @@ def set_social_auth_setting(self, setting_name, value): # does not rely on Django settings. self.strategy.set_settings({f'SOCIAL_AUTH_{backend_name}_{setting_name}': value}) - def access_token_body(self, request, _url, headers): + def access_token_body(self, request): """ Generates a response from the provider's access token endpoint. """ # The backend should always request JWT access tokens, not Bearer. - body = six.moves.urllib.parse.parse_qs(request.body.decode('utf8')) + body_content = request.body + if isinstance(body_content, bytes): + body_content = body_content.decode('utf8') + body = six.moves.urllib.parse.parse_qs(body_content) self.assertEqual(body['token_type'], ['jwt']) expires_in = 3600 @@ -51,7 +55,16 @@ def access_token_body(self, request, _url, headers): 'expires_in': expires_in, 'access_token': access_token }) - return 200, headers, body + return (200, {}, body) + + def pre_complete_callback(self, start_url): + """ Override to properly set up the access token response with callback. """ + responses.add_callback( + responses.POST, + url=self.backend.access_token_url(), + callback=self.access_token_body, + content_type="application/json", + ) def create_jwt_access_token(self, expires_in=3600, issuer=None, key=None, alg='RS512'): """ From 5c1717d9f15fa36f4139d885116bd3cec2a477d2 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 11 Aug 2025 12:39:57 +0000 Subject: [PATCH 12/21] refactor: update test descriptions for clarity and adjust responses requirement comment --- auth_backends/tests/test_pipeline.py | 154 ++++++++++++++------------- requirements/test.in | 2 +- 2 files changed, 79 insertions(+), 77 deletions(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 1cead18f..0c775181 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -24,83 +24,95 @@ def setUp(self): @ddt.data(True, False) def test_no_user_exists(self, setting_value): - """ - Verify an empty dict is returned if no user exists regardless of setting. - """ + """Returns empty dict if no user exists regardless of setting.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=setting_value): actual = get_user_if_exists(None, self.details) self.assertDictEqual(actual, {}) - @ddt.data( - # (test_config, expected_result) - ({'setting': True, 'user_provided': False, 'username_match': True, 'target_exists': True}, - {'is_new': False, 'user': 'found_user'}), # setting=True, no current user - ({'setting': False, 'user_provided': False, 'username_match': True, 'target_exists': True}, - {'is_new': False, 'user': 'found_user'}), # setting=False, no current user - ({'setting': True, 'user_provided': True, 'username_match': True, 'target_exists': True}, - {'is_new': False}), # setting=True, usernames match - ({'setting': False, 'user_provided': True, 'username_match': True, 'target_exists': True}, - {'is_new': False}), # setting=False, usernames match - ({'setting': True, 'user_provided': True, 'username_match': False, 'target_exists': True}, - {'is_new': False, 'user': 'target_user'}), # setting=True, mismatch with target - ({'setting': False, 'user_provided': True, 'username_match': False, 'target_exists': True}, - {'is_new': False}), # setting=False, mismatch with target - ({'setting': True, 'user_provided': True, 'username_match': False, 'target_exists': False}, - {}), # setting=True, mismatch no target - ) - @ddt.unpack + @ddt.data(True, False) @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') - def test_user_scenarios(self, test_config, expected_result, mock_set_attribute, mock_logger): - """ - Test various user scenarios with different settings and user conditions. - """ - setting_value = test_config['setting'] - user_provided = test_config['user_provided'] - username_match = test_config['username_match'] - target_exists = test_config['target_exists'] + def test_get_user_if_exists_no_current_user(self, toggle_enabled, mock_set_attribute, mock_logger): + """Returns found user when no current user exists, regardless of toggle setting.""" + with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): + found_user = User.objects.create(username=self.username) - with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=setting_value): - if user_provided: - user = User.objects.create(username='existing_user') - if username_match: - details = {'username': 'existing_user'} - else: - if target_exists: - target_user = User.objects.create(username='different_user') - details = {'username': 'different_user'} - - if expected_result.get('user') == 'target_user': - expected_result['user'] = target_user - else: - details = {'username': 'nonexistent_user'} - else: - user = None - found_user = User.objects.create(username=self.username) - details = self.details + actual = get_user_if_exists(None, self.details, user=None) - if expected_result.get('user') == 'found_user': - expected_result['user'] = found_user + expected = {'is_new': False, 'user': found_user} + self.assertDictEqual(actual, expected) + mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled) + mock_logger.info.assert_not_called() + + @ddt.data(True, False) + @patch('auth_backends.pipeline.logger') + @patch('auth_backends.pipeline.set_custom_attribute') + def test_get_user_if_exists_username_match(self, toggle_enabled, mock_set_attribute, mock_logger): + """Returns empty dict when current user matches details username, regardless of toggle.""" + with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): + user = User.objects.create(username='existing_user') + details = {'username': 'existing_user'} actual = get_user_if_exists(None, details, user=user) - self.assertDictEqual(actual, expected_result) - mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', setting_value) + self.assertDictEqual(actual, {'is_new': False}) + mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled) + mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', False) + mock_logger.info.assert_not_called() + + @ddt.data(True, False) + @patch('auth_backends.pipeline.logger') + @patch('auth_backends.pipeline.set_custom_attribute') + def test_get_user_if_exists_username_mismatch_with_target(self, toggle_enabled, mock_set_attribute, mock_logger): + """Toggle enabled: return target user. Toggle disabled: return empty dict.""" + with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): + user = User.objects.create(username='existing_user') + target_user = User.objects.create(username='different_user') + details = {'username': 'different_user'} - if user_provided: - mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', not username_match) + actual = get_user_if_exists(None, details, user=user) - if user_provided and not username_match and setting_value: - mock_logger.info.assert_called_once() - if not target_exists: - mock_logger.info.assert_called_with( - "Username mismatch detected. Username from Details: %s, Username from User: %s.", - 'nonexistent_user', - 'existing_user' - ) + if toggle_enabled: + expected = {'is_new': False, 'user': target_user} + mock_logger.info.assert_called_with( + "Username mismatch detected. Username from Details: %s, Username from User: %s.", + 'different_user', + 'existing_user' + ) else: + expected = {'is_new': False} mock_logger.info.assert_not_called() + self.assertDictEqual(actual, expected) + mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled) + mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', True) + + @ddt.data(True, False) + @patch('auth_backends.pipeline.logger') + @patch('auth_backends.pipeline.set_custom_attribute') + def test_get_user_if_exists_username_mismatch_no_target(self, toggle_enabled, mock_set_attribute, mock_logger): + """Toggle enabled: log warning and return empty dict. Toggle disabled: return empty dict without logging.""" + with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): + user = User.objects.create(username='existing_user') + details = {'username': 'nonexistent_user'} + + actual = get_user_if_exists(None, details, user=user) + + if toggle_enabled: + expected = {} + mock_logger.info.assert_called_with( + "Username mismatch detected. Username from Details: %s, Username from User: %s.", + 'nonexistent_user', + 'existing_user' + ) + else: + expected = {'is_new': False} + mock_logger.info.assert_not_called() + + self.assertDictEqual(actual, expected) + mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled) + mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', True) + class UpdateEmailPipelineTests(TestCase): """ @@ -115,9 +127,7 @@ def setUp(self): @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. - """ + """Updates user email when usernames match.""" mock_toggle.return_value = False updated_email = 'updated@example.com' self.assertNotEqual(self.user.email, updated_email) @@ -137,9 +147,7 @@ def test_update_email(self, mock_set_attribute, mock_toggle): @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. - """ + """Skips email update when email value is None.""" mock_toggle.return_value = False old_email = self.user.email @@ -156,9 +164,7 @@ def test_update_email_with_none(self, mock_set_attribute, mock_toggle): @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. - """ + """Skips email update when usernames don't match and toggle is enabled.""" mock_toggle.return_value = True old_email = self.user.email @@ -191,9 +197,7 @@ def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mo @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. - """ + """Updates email when usernames don't match but toggle is disabled.""" mock_toggle.return_value = False old_email = self.user.email @@ -215,9 +219,7 @@ def test_username_mismatch_with_update_toggle_disabled(self, mock_set_attribute, 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. - """ + """Asserts that a specific attribute was or was not set via set_custom_attribute.""" matching_calls = [ call for call in mock_set_attribute.call_args_list if call[0][0] == attribute_name diff --git a/requirements/test.in b/requirements/test.in index 198fe455..f3716d5e 100644 --- a/requirements/test.in +++ b/requirements/test.in @@ -11,7 +11,7 @@ pycodestyle pycryptodomex # used for crypto tests pytest-cov pytest-django -responses +responses # required by ddt tox typing_extensions unittest2 From c87ef89b5ec40124e18e4505ce5b1105398a2f2e Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Tue, 12 Aug 2025 06:56:50 +0000 Subject: [PATCH 13/21] refactor: enhance mock assertions and improve test descriptions for clarity --- auth_backends/tests/test_pipeline.py | 41 ++++++++++++++++++---------- 1 file changed, 26 insertions(+), 15 deletions(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 0c775181..8bb48d95 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -1,6 +1,6 @@ """ Tests for pipelines. """ -from unittest.mock import patch +from unittest.mock import patch, call import ddt from django.contrib.auth import get_user_model from django.test import TestCase, override_settings @@ -56,8 +56,10 @@ def test_get_user_if_exists_username_match(self, toggle_enabled, mock_set_attrib actual = get_user_if_exists(None, details, user=user) self.assertDictEqual(actual, {'is_new': False}) - mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled) - mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', False) + mock_set_attribute.assert_has_calls([ + call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled), + call('get_user_if_exists.username_mismatch', False) + ], any_order=True) mock_logger.info.assert_not_called() @ddt.data(True, False) @@ -84,8 +86,10 @@ def test_get_user_if_exists_username_mismatch_with_target(self, toggle_enabled, mock_logger.info.assert_not_called() self.assertDictEqual(actual, expected) - mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled) - mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', True) + mock_set_attribute.assert_has_calls([ + call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled), + call('get_user_if_exists.username_mismatch', True) + ], any_order=True) @ddt.data(True, False) @patch('auth_backends.pipeline.logger') @@ -110,14 +114,14 @@ def test_get_user_if_exists_username_mismatch_no_target(self, toggle_enabled, mo mock_logger.info.assert_not_called() self.assertDictEqual(actual, expected) - mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled) - mock_set_attribute.assert_any_call('get_user_if_exists.username_mismatch', True) + mock_set_attribute.assert_has_calls([ + call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled), + call('get_user_if_exists.username_mismatch', True) + ], any_order=True) class UpdateEmailPipelineTests(TestCase): - """ - Tests for the update_email pipeline function. - """ + """ Tests for the update_email pipeline function. """ def setUp(self): super().setUp() @@ -127,7 +131,7 @@ def setUp(self): @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): - """Updates user email when usernames match.""" + """ 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) @@ -147,7 +151,7 @@ def test_update_email(self, mock_set_attribute, mock_toggle): @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): - """Skips email update when email value is None.""" + """ Verify that user email is not updated if email value is None. """ mock_toggle.return_value = False old_email = self.user.email @@ -164,7 +168,7 @@ def test_update_email_with_none(self, mock_set_attribute, mock_toggle): @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): - """Skips email update when usernames don't match and toggle is enabled.""" + """ 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 @@ -197,7 +201,7 @@ def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mo @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): - """Updates email when usernames don't match but toggle is disabled.""" + """ Verify that email is updated when usernames don't match but toggle is disabled. """ mock_toggle.return_value = False old_email = self.user.email @@ -219,7 +223,14 @@ def test_username_mismatch_with_update_toggle_disabled(self, mock_set_attribute, 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): - """Asserts that a specific attribute was or was not set via set_custom_attribute.""" + """ + 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 From 08b79c4a6bede551356ebe64f4efe97b165935d3 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Tue, 12 Aug 2025 07:28:59 +0000 Subject: [PATCH 14/21] refactor: add comments to test cases for toggle and clarify expected results --- auth_backends/tests/test_pipeline.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 8bb48d95..a4a1fa74 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -22,14 +22,15 @@ def setUp(self): self.username = 'edx' self.details = {'username': self.username} - @ddt.data(True, False) + @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle def test_no_user_exists(self, setting_value): """Returns empty dict if no user exists regardless of setting.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=setting_value): actual = get_user_if_exists(None, self.details) - self.assertDictEqual(actual, {}) + expected = {} + self.assertDictEqual(actual, expected) - @ddt.data(True, False) + @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') def test_get_user_if_exists_no_current_user(self, toggle_enabled, mock_set_attribute, mock_logger): @@ -44,7 +45,7 @@ def test_get_user_if_exists_no_current_user(self, toggle_enabled, mock_set_attri mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled) mock_logger.info.assert_not_called() - @ddt.data(True, False) + @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') def test_get_user_if_exists_username_match(self, toggle_enabled, mock_set_attribute, mock_logger): @@ -54,15 +55,16 @@ def test_get_user_if_exists_username_match(self, toggle_enabled, mock_set_attrib details = {'username': 'existing_user'} actual = get_user_if_exists(None, details, user=user) + expected = {'is_new': False} - self.assertDictEqual(actual, {'is_new': False}) + self.assertDictEqual(actual, expected) mock_set_attribute.assert_has_calls([ call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled), call('get_user_if_exists.username_mismatch', False) ], any_order=True) mock_logger.info.assert_not_called() - @ddt.data(True, False) + @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') def test_get_user_if_exists_username_mismatch_with_target(self, toggle_enabled, mock_set_attribute, mock_logger): @@ -91,7 +93,7 @@ def test_get_user_if_exists_username_mismatch_with_target(self, toggle_enabled, call('get_user_if_exists.username_mismatch', True) ], any_order=True) - @ddt.data(True, False) + @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') def test_get_user_if_exists_username_mismatch_no_target(self, toggle_enabled, mock_set_attribute, mock_logger): From e59b78779c63648321a70ecf666d57a768214485 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Tue, 12 Aug 2025 09:25:43 +0000 Subject: [PATCH 15/21] refactor: simplify docstring formatting and enhance test cases for username mismatch scenarios --- auth_backends/pipeline.py | 4 +- auth_backends/tests/test_pipeline.py | 55 +++++++++++++--------------- 2 files changed, 26 insertions(+), 33 deletions(-) diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index 0a723be1..8f788d79 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -81,9 +81,7 @@ 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. - """ + """Update the user's email address using data from provider.""" if user: # Get usernames for comparison, using defensive coding diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index a4a1fa74..abef2b88 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -64,54 +64,49 @@ def test_get_user_if_exists_username_match(self, toggle_enabled, mock_set_attrib ], any_order=True) mock_logger.info.assert_not_called() - @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle + @ddt.data( + (True, True), # toggle enabled, target exists + (True, False), # toggle enabled, target missing + (False, True), # toggle disabled, target exists + (False, False) # toggle disabled, target missing + ) + @ddt.unpack @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') - def test_get_user_if_exists_username_mismatch_with_target(self, toggle_enabled, mock_set_attribute, mock_logger): - """Toggle enabled: return target user. Toggle disabled: return empty dict.""" + def test_get_user_if_exists_username_mismatch(self, toggle_enabled, target_exists, mock_set_attribute, mock_logger): + """Test username mismatch scenarios with different toggle states and target user presence.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): user = User.objects.create(username='existing_user') - target_user = User.objects.create(username='different_user') - details = {'username': 'different_user'} + target_user = None + + if target_exists: + target_user = User.objects.create(username='different_user') + details = {'username': 'different_user'} + details_username = 'different_user' + else: + details = {'username': 'nonexistent_user'} + details_username = 'nonexistent_user' actual = get_user_if_exists(None, details, user=user) - if toggle_enabled: + if toggle_enabled and target_exists: + # Toggle enabled and target exists: return target user expected = {'is_new': False, 'user': target_user} mock_logger.info.assert_called_with( "Username mismatch detected. Username from Details: %s, Username from User: %s.", - 'different_user', + details_username, 'existing_user' ) - else: - expected = {'is_new': False} - mock_logger.info.assert_not_called() - - self.assertDictEqual(actual, expected) - mock_set_attribute.assert_has_calls([ - call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled), - call('get_user_if_exists.username_mismatch', True) - ], any_order=True) - - @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle - @patch('auth_backends.pipeline.logger') - @patch('auth_backends.pipeline.set_custom_attribute') - def test_get_user_if_exists_username_mismatch_no_target(self, toggle_enabled, mock_set_attribute, mock_logger): - """Toggle enabled: log warning and return empty dict. Toggle disabled: return empty dict without logging.""" - with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): - user = User.objects.create(username='existing_user') - details = {'username': 'nonexistent_user'} - - actual = get_user_if_exists(None, details, user=user) - - if toggle_enabled: + elif toggle_enabled and not target_exists: + # Toggle enabled and no target: return empty dict with logging expected = {} mock_logger.info.assert_called_with( "Username mismatch detected. Username from Details: %s, Username from User: %s.", - 'nonexistent_user', + details_username, 'existing_user' ) else: + # Toggle disabled: return empty dict without logging expected = {'is_new': False} mock_logger.info.assert_not_called() From 40ce3680729b986d3db73e079cb4afd9d8a83696 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Thu, 14 Aug 2025 10:01:31 +0000 Subject: [PATCH 16/21] refactor: improve test descriptions for user existence scenarios and clarify toggle behavior --- auth_backends/tests/test_pipeline.py | 70 +++++++++++++++++----------- 1 file changed, 42 insertions(+), 28 deletions(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index abef2b88..0f935ddf 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -22,7 +22,8 @@ def setUp(self): self.username = 'edx' self.details = {'username': self.username} - @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle + # Test behavior when no user exists in database with and without toggle + @ddt.data(True, False) def test_no_user_exists(self, setting_value): """Returns empty dict if no user exists regardless of setting.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=setting_value): @@ -30,7 +31,8 @@ def test_no_user_exists(self, setting_value): expected = {} self.assertDictEqual(actual, expected) - @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle + # Test behavior when user exists but no current user with and without toggle + @ddt.data(True, False) @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') def test_get_user_if_exists_no_current_user(self, toggle_enabled, mock_set_attribute, mock_logger): @@ -45,7 +47,8 @@ def test_get_user_if_exists_no_current_user(self, toggle_enabled, mock_set_attri mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled) mock_logger.info.assert_not_called() - @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle + # Test behavior when usernames match with and without toggle + @ddt.data(True, False) @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') def test_get_user_if_exists_username_match(self, toggle_enabled, mock_set_attribute, mock_logger): @@ -64,49 +67,60 @@ def test_get_user_if_exists_username_match(self, toggle_enabled, mock_set_attrib ], any_order=True) mock_logger.info.assert_not_called() - @ddt.data( - (True, True), # toggle enabled, target exists - (True, False), # toggle enabled, target missing - (False, True), # toggle disabled, target exists - (False, False) # toggle disabled, target missing - ) - @ddt.unpack + # Test username mismatch when target user exists with and without toggle + @ddt.data(True, False) @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') - def test_get_user_if_exists_username_mismatch(self, toggle_enabled, target_exists, mock_set_attribute, mock_logger): - """Test username mismatch scenarios with different toggle states and target user presence.""" + def test_get_user_if_exists_username_mismatch_with_target(self, toggle_enabled, mock_set_attribute, mock_logger): + """Toggle enabled: return target user. Toggle disabled: return empty dict.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): user = User.objects.create(username='existing_user') - target_user = None + user_map = {} - if target_exists: - target_user = User.objects.create(username='different_user') - details = {'username': 'different_user'} - details_username = 'different_user' - else: - details = {'username': 'nonexistent_user'} - details_username = 'nonexistent_user' + target_user = User.objects.create(username='different_user') + user_map['target_user'] = target_user + + details = {'username': 'different_user'} actual = get_user_if_exists(None, details, user=user) - if toggle_enabled and target_exists: - # Toggle enabled and target exists: return target user - expected = {'is_new': False, 'user': target_user} + if toggle_enabled: + expected = {'is_new': False, 'user': user_map['target_user']} mock_logger.info.assert_called_with( "Username mismatch detected. Username from Details: %s, Username from User: %s.", - details_username, + 'different_user', 'existing_user' ) - elif toggle_enabled and not target_exists: - # Toggle enabled and no target: return empty dict with logging + else: + expected = {'is_new': False} + mock_logger.info.assert_not_called() + + self.assertDictEqual(actual, expected) + mock_set_attribute.assert_has_calls([ + call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled), + call('get_user_if_exists.username_mismatch', True) + ], any_order=True) + + # Test username mismatch when target user does not exist with and without toggle + @ddt.data(True, False) + @patch('auth_backends.pipeline.logger') + @patch('auth_backends.pipeline.set_custom_attribute') + def test_get_user_if_exists_username_mismatch_no_target(self, toggle_enabled, mock_set_attribute, mock_logger): + """Toggle enabled: log warning and return empty dict. Toggle disabled: return empty dict without logging.""" + with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): + user = User.objects.create(username='existing_user') + details = {'username': 'nonexistent_user'} + + actual = get_user_if_exists(None, details, user=user) + + if toggle_enabled: expected = {} mock_logger.info.assert_called_with( "Username mismatch detected. Username from Details: %s, Username from User: %s.", - details_username, + 'nonexistent_user', 'existing_user' ) else: - # Toggle disabled: return empty dict without logging expected = {'is_new': False} mock_logger.info.assert_not_called() From 989ae1148f8670cba803b9fd1b23dd4f1adfa7ac Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Thu, 14 Aug 2025 11:46:28 +0000 Subject: [PATCH 17/21] refactor: update test descriptions for clarity in user existence scenarios --- auth_backends/tests/test_pipeline.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 0f935ddf..4f72b172 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -22,7 +22,7 @@ def setUp(self): self.username = 'edx' self.details = {'username': self.username} - # Test behavior when no user exists in database with and without toggle + # Test no user exists in database with and without toggle @ddt.data(True, False) def test_no_user_exists(self, setting_value): """Returns empty dict if no user exists regardless of setting.""" @@ -31,7 +31,7 @@ def test_no_user_exists(self, setting_value): expected = {} self.assertDictEqual(actual, expected) - # Test behavior when user exists but no current user with and without toggle + # Test user exists but no current user with and without toggle @ddt.data(True, False) @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') @@ -47,7 +47,7 @@ def test_get_user_if_exists_no_current_user(self, toggle_enabled, mock_set_attri mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled) mock_logger.info.assert_not_called() - # Test behavior when usernames match with and without toggle + # Test usernames match with and without toggle @ddt.data(True, False) @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') From 218889e6598e71e43177502802370cb9010ed6b4 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Thu, 21 Aug 2025 12:03:53 +0000 Subject: [PATCH 18/21] refactor: improve test descriptions and formatting for clarity in user existence scenarios --- auth_backends/tests/test_pipeline.py | 45 +++++++++++++--------------- requirements/test.in | 10 +++---- 2 files changed, 25 insertions(+), 30 deletions(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 4f72b172..43c981de 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -22,23 +22,21 @@ def setUp(self): self.username = 'edx' self.details = {'username': self.username} - # Test no user exists in database with and without toggle - @ddt.data(True, False) - def test_no_user_exists(self, setting_value): + @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle enabled/disabled + def test_no_user_exists(self, toggle_enabled): """Returns empty dict if no user exists regardless of setting.""" - with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=setting_value): + with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): actual = get_user_if_exists(None, self.details) expected = {} self.assertDictEqual(actual, expected) - # Test user exists but no current user with and without toggle - @ddt.data(True, False) + @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle enabled/disabled @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') def test_get_user_if_exists_no_current_user(self, toggle_enabled, mock_set_attribute, mock_logger): - """Returns found user when no current user exists, regardless of toggle setting.""" + """Returns details user when it can be found and there is no current user, regardless of toggle setting""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): - found_user = User.objects.create(username=self.username) + found_user = User.objects.create(username=self.details['username']) actual = get_user_if_exists(None, self.details, user=None) @@ -47,12 +45,11 @@ def test_get_user_if_exists_no_current_user(self, toggle_enabled, mock_set_attri mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled) mock_logger.info.assert_not_called() - # Test usernames match with and without toggle - @ddt.data(True, False) + @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle enabled/disabled @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') def test_get_user_if_exists_username_match(self, toggle_enabled, mock_set_attribute, mock_logger): - """Returns empty dict when current user matches details username, regardless of toggle.""" + """Returns dict without user element when current user matches details username, regardless of toggle.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): user = User.objects.create(username='existing_user') details = {'username': 'existing_user'} @@ -67,25 +64,22 @@ def test_get_user_if_exists_username_match(self, toggle_enabled, mock_set_attrib ], any_order=True) mock_logger.info.assert_not_called() - # Test username mismatch when target user exists with and without toggle - @ddt.data(True, False) + @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle enabled/disabled @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') - def test_get_user_if_exists_username_mismatch_with_target(self, toggle_enabled, mock_set_attribute, mock_logger): - """Toggle enabled: return target user. Toggle disabled: return empty dict.""" + def test_get_user_if_exists_username_mismatch_and_details_user_found( + self, toggle_enabled, mock_set_attribute, mock_logger + ): + """Toggle enabled: return found details user. Toggle disabled: return dict without user element.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): user = User.objects.create(username='existing_user') - user_map = {} - - target_user = User.objects.create(username='different_user') - user_map['target_user'] = target_user - details = {'username': 'different_user'} + found_user = User.objects.create(username=details['username']) actual = get_user_if_exists(None, details, user=user) if toggle_enabled: - expected = {'is_new': False, 'user': user_map['target_user']} + expected = {'is_new': False, 'user': found_user} mock_logger.info.assert_called_with( "Username mismatch detected. Username from Details: %s, Username from User: %s.", 'different_user', @@ -101,12 +95,13 @@ def test_get_user_if_exists_username_mismatch_with_target(self, toggle_enabled, call('get_user_if_exists.username_mismatch', True) ], any_order=True) - # Test username mismatch when target user does not exist with and without toggle - @ddt.data(True, False) + @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle enabled/disabled @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') - def test_get_user_if_exists_username_mismatch_no_target(self, toggle_enabled, mock_set_attribute, mock_logger): - """Toggle enabled: log warning and return empty dict. Toggle disabled: return empty dict without logging.""" + def test_get_user_if_exists_username_mismatch_details_user_not_found( + self, toggle_enabled, mock_set_attribute, mock_logger + ): + """Toggle enabled: return empty dict. Toggle disabled: return dict without user element.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): user = User.objects.create(username='existing_user') details = {'username': 'nonexistent_user'} diff --git a/requirements/test.in b/requirements/test.in index f3716d5e..5fb7bc70 100644 --- a/requirements/test.in +++ b/requirements/test.in @@ -4,15 +4,15 @@ -r base.txt # Core dependencies coverage -ddt # for running multiple test cases with multiple input +ddt # for running multiple test cases with multiple input edx-lint httpretty pycodestyle -pycryptodomex # used for crypto tests +pycryptodomex # used for crypto tests pytest-cov pytest-django -responses # required by ddt +responses # required by ddt tox -typing_extensions +typing_extensions # required by ddt unittest2 -edx-django-release-util # Contains the reserved keyword check +edx-django-release-util # Contains the reserved keyword check From 660866f3da87099e5f7885d0a3b7055b42ebf0dc Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 25 Aug 2025 09:01:57 +0000 Subject: [PATCH 19/21] refactor: flatten the details dictionary --- auth_backends/tests/test_pipeline.py | 35 ++++++++++++++-------------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 43c981de..ed9a9918 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -19,14 +19,16 @@ class GetUserIfExistsPipelineTests(TestCase): def setUp(self): super().setUp() - self.username = 'edx' - self.details = {'username': self.username} + self.details_for_existing_user = {'username': 'existing_user'} + self.details_for_non_existing_user = {'username': 'non_existing_user'} + self.details_for_different_user = {'username': 'different_user'} + self.user = User.objects.create(**self.details_for_existing_user) @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle enabled/disabled def test_no_user_exists(self, toggle_enabled): """Returns empty dict if no user exists regardless of setting.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): - actual = get_user_if_exists(None, self.details) + actual = get_user_if_exists(None, self.details_for_non_existing_user) expected = {} self.assertDictEqual(actual, expected) @@ -36,11 +38,11 @@ def test_no_user_exists(self, toggle_enabled): def test_get_user_if_exists_no_current_user(self, toggle_enabled, mock_set_attribute, mock_logger): """Returns details user when it can be found and there is no current user, regardless of toggle setting""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): - found_user = User.objects.create(username=self.details['username']) + existing_user = self.user - actual = get_user_if_exists(None, self.details, user=None) + actual = get_user_if_exists(None, self.details_for_existing_user, user=None) - expected = {'is_new': False, 'user': found_user} + expected = {'is_new': False, 'user': existing_user} self.assertDictEqual(actual, expected) mock_set_attribute.assert_any_call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled) mock_logger.info.assert_not_called() @@ -51,10 +53,9 @@ def test_get_user_if_exists_no_current_user(self, toggle_enabled, mock_set_attri def test_get_user_if_exists_username_match(self, toggle_enabled, mock_set_attribute, mock_logger): """Returns dict without user element when current user matches details username, regardless of toggle.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): - user = User.objects.create(username='existing_user') - details = {'username': 'existing_user'} + existing_user = self.user - actual = get_user_if_exists(None, details, user=user) + actual = get_user_if_exists(None, self.details_for_existing_user, user=existing_user) expected = {'is_new': False} self.assertDictEqual(actual, expected) @@ -72,14 +73,13 @@ def test_get_user_if_exists_username_mismatch_and_details_user_found( ): """Toggle enabled: return found details user. Toggle disabled: return dict without user element.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): - user = User.objects.create(username='existing_user') - details = {'username': 'different_user'} - found_user = User.objects.create(username=details['username']) + existing_user = self.user + different_user = User.objects.create(**self.details_for_different_user) - actual = get_user_if_exists(None, details, user=user) + actual = get_user_if_exists(None, self.details_for_different_user, user=existing_user) if toggle_enabled: - expected = {'is_new': False, 'user': found_user} + expected = {'is_new': False, 'user': different_user} mock_logger.info.assert_called_with( "Username mismatch detected. Username from Details: %s, Username from User: %s.", 'different_user', @@ -103,16 +103,15 @@ def test_get_user_if_exists_username_mismatch_details_user_not_found( ): """Toggle enabled: return empty dict. Toggle disabled: return dict without user element.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): - user = User.objects.create(username='existing_user') - details = {'username': 'nonexistent_user'} + existing_user = self.user - actual = get_user_if_exists(None, details, user=user) + actual = get_user_if_exists(None, self.details_for_non_existing_user, user=existing_user) if toggle_enabled: expected = {} mock_logger.info.assert_called_with( "Username mismatch detected. Username from Details: %s, Username from User: %s.", - 'nonexistent_user', + 'non_existing_user', 'existing_user' ) else: From 9305b4c2886b2115c054a219e29b20173cf91e81 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Tue, 26 Aug 2025 14:04:49 +0000 Subject: [PATCH 20/21] refactor: update CHANGELOG and enhance user authentication pipeline with custom attributes for username checks --- CHANGELOG.rst | 7 ++++--- auth_backends/pipeline.py | 6 ++++++ auth_backends/tests/test_pipeline.py | 15 ++++++++------- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 1e09a9d2..76822ae0 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,15 +13,16 @@ Unreleased ---------- -[4.6.1] - 2025-07-25 +[4.6.1] - 2025-08-26 -------------------- Fixed ~~~~~ -* Fixed authentication issues where username mismatches between logged-in users and social auth details caused "user already exists" errors. +* Fixed authentication issues where the social auth details username is not being respected as the user to be logged in in certain cases. + When the already logged-in user does not match the social auth details user, the pipeline should log in the new user, rather than leaving some other user logged in. - * Added temporary rollout toggle IGNORE_LOGGED_IN_USER_ON_MISMATCH to ensure the bug fix doesn't introduce any issues.. + * Added temporary rollout toggle IGNORE_LOGGED_IN_USER_ON_MISMATCH to ensure the bug fix doesn't introduce any issues. * Enhanced get_user_if_exists function with monitoring capabilities for debugging authentication conflicts. [4.6.0] - 2025-06-18 diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index 8f788d79..9c848597 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -51,6 +51,12 @@ def get_user_if_exists(strategy, details, user=None, *args, **kwargs): # pylint # Check for username mismatch and toggle behavior details_username = details.get('username') user_username = getattr(user, 'username', None) + + # .. custom_attribute_name: get_user_if_exists.has_details_username + # .. custom_attribute_description: Tracks whether the details contain a username field. + # True if username is present and not None, False if username is missing or None. + set_custom_attribute('get_user_if_exists.has_details_username', bool(details_username)) + username_mismatch = details_username != user_username # .. custom_attribute_name: get_user_if_exists.username_mismatch diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index ed9a9918..d9d0cfb2 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -22,7 +22,7 @@ def setUp(self): self.details_for_existing_user = {'username': 'existing_user'} self.details_for_non_existing_user = {'username': 'non_existing_user'} self.details_for_different_user = {'username': 'different_user'} - self.user = User.objects.create(**self.details_for_existing_user) + self.existing_user = User.objects.create(**self.details_for_existing_user) @ddt.data(True, False) # test IGNORE_LOGGED_IN_USER_ON_MISMATCH toggle enabled/disabled def test_no_user_exists(self, toggle_enabled): @@ -38,8 +38,7 @@ def test_no_user_exists(self, toggle_enabled): def test_get_user_if_exists_no_current_user(self, toggle_enabled, mock_set_attribute, mock_logger): """Returns details user when it can be found and there is no current user, regardless of toggle setting""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): - existing_user = self.user - + existing_user = self.existing_user actual = get_user_if_exists(None, self.details_for_existing_user, user=None) expected = {'is_new': False, 'user': existing_user} @@ -53,14 +52,14 @@ def test_get_user_if_exists_no_current_user(self, toggle_enabled, mock_set_attri def test_get_user_if_exists_username_match(self, toggle_enabled, mock_set_attribute, mock_logger): """Returns dict without user element when current user matches details username, regardless of toggle.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): - existing_user = self.user - + existing_user = self.existing_user actual = get_user_if_exists(None, self.details_for_existing_user, user=existing_user) expected = {'is_new': False} self.assertDictEqual(actual, expected) mock_set_attribute.assert_has_calls([ call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled), + call('get_user_if_exists.has_details_username', True), call('get_user_if_exists.username_mismatch', False) ], any_order=True) mock_logger.info.assert_not_called() @@ -73,7 +72,7 @@ def test_get_user_if_exists_username_mismatch_and_details_user_found( ): """Toggle enabled: return found details user. Toggle disabled: return dict without user element.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): - existing_user = self.user + existing_user = self.existing_user different_user = User.objects.create(**self.details_for_different_user) actual = get_user_if_exists(None, self.details_for_different_user, user=existing_user) @@ -92,6 +91,7 @@ def test_get_user_if_exists_username_mismatch_and_details_user_found( self.assertDictEqual(actual, expected) mock_set_attribute.assert_has_calls([ call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled), + call('get_user_if_exists.has_details_username', True), call('get_user_if_exists.username_mismatch', True) ], any_order=True) @@ -103,7 +103,7 @@ def test_get_user_if_exists_username_mismatch_details_user_not_found( ): """Toggle enabled: return empty dict. Toggle disabled: return dict without user element.""" with override_settings(IGNORE_LOGGED_IN_USER_ON_MISMATCH=toggle_enabled): - existing_user = self.user + existing_user = self.existing_user actual = get_user_if_exists(None, self.details_for_non_existing_user, user=existing_user) @@ -121,6 +121,7 @@ def test_get_user_if_exists_username_mismatch_details_user_not_found( self.assertDictEqual(actual, expected) mock_set_attribute.assert_has_calls([ call('get_user_if_exists.ignore_toggle_enabled', toggle_enabled), + call('get_user_if_exists.has_details_username', True), call('get_user_if_exists.username_mismatch', True) ], any_order=True) From fb0ca7e5fd82519a5f230caf70451932c8b93348 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Wed, 27 Aug 2025 10:46:18 +0530 Subject: [PATCH 21/21] fix: Update auth_backends/pipeline.py comment Co-authored-by: Robert Raposa --- auth_backends/pipeline.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index 9c848597..b402ae84 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -54,7 +54,7 @@ def get_user_if_exists(strategy, details, user=None, *args, **kwargs): # pylint # .. custom_attribute_name: get_user_if_exists.has_details_username # .. custom_attribute_description: Tracks whether the details contain a username field. - # True if username is present and not None, False if username is missing or None. + # True if username is present, False if missing. set_custom_attribute('get_user_if_exists.has_details_username', bool(details_username)) username_mismatch = details_username != user_username