diff --git a/CHANGELOG.rst b/CHANGELOG.rst index a6233d8..7f4cb08 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,14 @@ Unreleased ---------- +[4.6.2] - 2025-10-16 +-------------------- + +Changed +~~~~~~~ + +* Removed temporary rollout toggles and simplified custom attribute tracking after successful OAuth session cleanup rollout. + [4.6.1] - 2025-09-23 -------------------- diff --git a/auth_backends/__init__.py b/auth_backends/__init__.py index 34383a1..e3d0f3d 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.1' # pragma: no cover +__version__ = '4.6.2' # pragma: no cover diff --git a/auth_backends/backends.py b/auth_backends/backends.py index d9a3ba0..c2d7675 100644 --- a/auth_backends/backends.py +++ b/auth_backends/backends.py @@ -7,23 +7,10 @@ from django.contrib.auth import logout from django.dispatch import Signal from social_core.backends.oauth import BaseOAuth2 -from edx_toggles.toggles import SettingToggle from edx_django_utils.monitoring import set_custom_attribute logger = logging.getLogger(__name__) -# .. toggle_name: ENABLE_OAUTH_SESSION_CLEANUP -# .. toggle_implementation: SettingToggle -# .. toggle_default: False -# .. toggle_description: Controls whether to perform session cleanup during OAuth start. -# When enabled (True), existing user sessions are cleared before OAuth authentication -# to prevent user association conflicts. When disabled (False), session cleanup is skipped. -# This toggle allows for gradual rollout and quick rollback if issues arise. -# .. toggle_use_cases: temporary -# .. toggle_creation_date: 2025-09-25 -# .. toggle_target_removal_date: 2025-11-25 -ENABLE_OAUTH_SESSION_CLEANUP = SettingToggle("ENABLE_OAUTH_SESSION_CLEANUP", default=False) - PROFILE_CLAIMS_TO_DETAILS_KEY_MAP = { 'preferred_username': 'username', 'email': 'email', @@ -88,20 +75,10 @@ def logout_url(self): return self.end_session_url() def start(self): - """Initialize OAuth authentication with optional session cleanup.""" - - # .. custom_attribute_name: session_cleanup.toggle_enabled - # .. custom_attribute_description: Tracks whether the ENABLE_OAUTH_SESSION_CLEANUP - # toggle is enabled during OAuth start. - set_custom_attribute('session_cleanup.toggle_enabled', ENABLE_OAUTH_SESSION_CLEANUP.is_enabled()) + """Initialize OAuth authentication with session cleanup.""" request = self.strategy.request if hasattr(self.strategy, 'request') else None - # .. custom_attribute_name: session_cleanup.has_request - # .. custom_attribute_description: Tracks whether a request object is available - # during OAuth start. True if request exists, False if missing. - set_custom_attribute('session_cleanup.has_request', request is not None) - user_authenticated = ( request is not None and hasattr(request, 'user') and @@ -113,14 +90,9 @@ def start(self): # before session cleanup. True if user was logged in, False otherwise. set_custom_attribute('session_cleanup.logout_required', user_authenticated) - if user_authenticated and ENABLE_OAUTH_SESSION_CLEANUP.is_enabled(): + if user_authenticated: existing_username = getattr(request.user, 'username', 'unknown') - # .. custom_attribute_name: session_cleanup.logged_out_username - # .. custom_attribute_description: Records the username that was logged out - # during session cleanup for tracking and debugging purposes. - set_custom_attribute('session_cleanup.logged_out_username', existing_username) - logger.info( "OAuth start: Performing session cleanup for user '%s'", existing_username @@ -128,13 +100,6 @@ def start(self): logout(request) - # .. custom_attribute_name: session_cleanup.logout_performed - # .. custom_attribute_description: Indicates that session cleanup was - # actually performed during OAuth start. - set_custom_attribute('session_cleanup.logout_performed', True) - else: - set_custom_attribute('session_cleanup.logout_performed', False) - return super().start() def authorization_url(self): diff --git a/auth_backends/pipeline.py b/auth_backends/pipeline.py index d92259e..a00233b 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -4,24 +4,11 @@ """ import logging from django.contrib.auth import get_user_model -from edx_toggles.toggles import SettingToggle from edx_django_utils.monitoring import set_custom_attribute logger = logging.getLogger(__name__) User = get_user_model() -# .. toggle_name: SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH -# .. toggle_implementation: SettingToggle -# .. toggle_default: False -# .. toggle_description: Determines whether to block email updates when usernames don't match. -# When enabled (True), email updates will be blocked when the username in social auth details -# doesn't match the user's username. When disabled (False), email updates will proceed regardless -# of username mismatches. This will be used for a temporary rollout. -# .. toggle_use_cases: temporary -# .. toggle_creation_date: 2025-06-18 -# .. toggle_target_removal_date: 2025-08-18 -SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH = SettingToggle("SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH", default=False) - # pylint: disable=unused-argument # The function parameters must be named exactly as they are below. @@ -56,49 +43,17 @@ def update_email(strategy, details, user=None, *args, **kwargs): # pylint: disa # Check if usernames don't match username_mismatch = details_username != user_username - # .. custom_attribute_name: update_email.username_mismatch - # .. custom_attribute_description: Tracks whether there's a mismatch between - # the username in the social details and the user's actual username. - # True if usernames don't match, False if they match. - set_custom_attribute('update_email.username_mismatch', username_mismatch) - - # .. custom_attribute_name: update_email.rollout_toggle_enabled - # .. custom_attribute_description: Tracks whether the SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH - # toggle is enabled during this pipeline execution. - set_custom_attribute('update_email.rollout_toggle_enabled', SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled()) - if username_mismatch: - # Log warning and set additional custom attributes for mismatches logger.warning( - "Username mismatch during email update. User username: %s, Details username: %s", + "Unexpected username mismatch during email update. Skipping email update for user %s. " + "User username: %s, Details username: %s", + user_username, user_username, details_username ) - # .. custom_attribute_name: update_email.details_username - # .. custom_attribute_description: Records the username provided in the - # social details when a mismatch occurs with the user's username. - set_custom_attribute('update_email.details_username', details_username) - - # .. custom_attribute_name: update_email.user_username - # .. custom_attribute_description: Records the actual username of the user - # when a mismatch occurs with the social details username. - set_custom_attribute('update_email.user_username', user_username) - - # .. custom_attribute_name: update_email.details_has_email - # .. custom_attribute_description: Records whether the details contain an email - # when a username mismatch occurs, to identify potential edge cases. - set_custom_attribute('update_email.details_has_email', bool(details.get('email'))) - - # Only exit if the toggle is enabled - if SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled(): - logger.warning( - "Skipping email update for user %s due to username mismatch and " - "SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH toggle enabled", - user_username - ) - return # Exit without updating email + return # Exit without updating email - # Proceed with email update only if usernames match or toggle is disabled + # Proceed with email update only if usernames match email = details.get('email') if email and user.email != email: user.email = email diff --git a/auth_backends/tests/test_backends.py b/auth_backends/tests/test_backends.py index f766685..e73237f 100644 --- a/auth_backends/tests/test_backends.py +++ b/auth_backends/tests/test_backends.py @@ -2,7 +2,7 @@ import datetime import json from calendar import timegm -from unittest.mock import patch, call +from unittest.mock import patch import ddt import jwt @@ -14,7 +14,6 @@ from django.contrib.sessions.middleware import SessionMiddleware from django.core.cache import cache from django.test import RequestFactory -from django.test.utils import override_settings from social_core.tests.backends.oauth import OAuth2Test User = get_user_model() @@ -127,54 +126,38 @@ def test_login(self): self.do_login() @pytest.mark.django_db - @ddt.data(True, False) # Test session cleanup with both toggle enabled and disabled + @ddt.data(True, False) # Test with and without authenticated user @patch('auth_backends.backends.set_custom_attribute') @patch('auth_backends.backends.logger') - def test_start_with_session_cleanup(self, toggle_enabled, mock_logger, mock_set_attr): - """Test start method for session cleanup of existing user with toggle variation.""" - with override_settings(ENABLE_OAUTH_SESSION_CLEANUP=toggle_enabled): - existing_user = User.objects.create_user(username='existing_user', email='existing@example.com') + def test_start_with_session_cleanup(self, user_authenticated, mock_logger, mock_set_attr): + """Test start method for session cleanup with and without authenticated user.""" + request = RequestFactory().get('/auth/login/edx-oauth2/') - request = RequestFactory().get('/auth/login/edx-oauth2/') + if user_authenticated: + existing_user = User.objects.create_user(username='existing_user', email='existing@example.com') request.user = existing_user - middleware = SessionMiddleware(lambda req: None) - middleware.process_request(request) - request.session.save() - - initial_session_key = request.session.session_key - - self.backend.strategy.request = request - - self.do_start() - - if toggle_enabled: - self.assertNotEqual(request.session.session_key, initial_session_key) - - self.assertTrue(request.user.is_anonymous) + middleware = SessionMiddleware(lambda req: None) + middleware.process_request(request) + request.session.save() - mock_set_attr.assert_has_calls([ - call('session_cleanup.toggle_enabled', True), - call('session_cleanup.logout_performed', True), - call('session_cleanup.logged_out_username', 'existing_user') - ], any_order=True) + initial_session_key = request.session.session_key - mock_logger.info.assert_called_with( - "OAuth start: Performing session cleanup for user '%s'", - 'existing_user' - ) - else: - self.assertEqual(request.session.session_key, initial_session_key) + self.backend.strategy.request = request - self.assertEqual(request.user, existing_user) - self.assertFalse(request.user.is_anonymous) + self.do_start() - mock_set_attr.assert_has_calls([ - call('session_cleanup.toggle_enabled', False), - call('session_cleanup.logout_performed', False) - ], any_order=True) + mock_set_attr.assert_called_once_with('session_cleanup.logout_required', user_authenticated) - mock_logger.info.assert_not_called() + if user_authenticated: + self.assertNotEqual(request.session.session_key, initial_session_key) + self.assertTrue(request.user.is_anonymous) + mock_logger.info.assert_called_with( + "OAuth start: Performing session cleanup for user '%s'", + 'existing_user' + ) + else: + mock_logger.info.assert_not_called() def test_partial_pipeline(self): self.do_partial_pipeline() diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 060dcb3..448a0fc 100644 --- a/auth_backends/tests/test_pipeline.py +++ b/auth_backends/tests/test_pipeline.py @@ -44,11 +44,9 @@ def setUp(self): self.user = User.objects.create(username='test_user') self.strategy = load_strategy() - @patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled') @patch('auth_backends.pipeline.set_custom_attribute') - def test_update_email(self, mock_set_attribute, mock_toggle): + def test_update_email(self, mock_set_attribute): """ 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) @@ -60,15 +58,11 @@ def test_update_email(self, mock_set_attribute, mock_toggle): self.assertEqual(updated_user.email, updated_email) self.assertNotEqual(updated_user.email, initial_email) - mock_set_attribute.assert_any_call('update_email.username_mismatch', False) - mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False) self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=True) - @patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled') @patch('auth_backends.pipeline.set_custom_attribute') - def test_update_email_with_none(self, mock_set_attribute, mock_toggle): + def test_update_email_with_none(self, mock_set_attribute): """ Verify that user email is not updated if email value is None. """ - mock_toggle.return_value = False old_email = self.user.email update_email(self.strategy, {'email': None, 'username': 'test_user'}, user=self.user) @@ -76,17 +70,12 @@ def test_update_email_with_none(self, mock_set_attribute, mock_toggle): updated_user = User.objects.get(pk=self.user.pk) self.assertEqual(updated_user.email, old_email) - mock_set_attribute.assert_any_call('update_email.username_mismatch', False) - mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False) self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=False) - @patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled') @patch('auth_backends.pipeline.logger') @patch('auth_backends.pipeline.set_custom_attribute') - def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mock_logger, mock_toggle): - """ Verify that email is not updated when usernames don't match and toggle is enabled. """ - mock_toggle.return_value = True - + def test_username_mismatch_no_update(self, mock_set_attribute, mock_logger): + """ Verify that email is not updated when usernames don't match. """ old_email = self.user.email updated_email = 'updated@example.com' @@ -95,49 +84,16 @@ def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mo updated_user = User.objects.get(pk=self.user.pk) self.assertEqual(updated_user.email, old_email) - self.assertEqual(mock_logger.warning.call_count, 2) - mock_logger.warning.assert_any_call( - "Username mismatch during email update. User username: %s, Details username: %s", - 'test_user', 'different_user' - ) - mock_logger.warning.assert_any_call( - "Skipping email update for user %s due to username mismatch and " - "SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH toggle enabled", - 'test_user' + mock_logger.warning.assert_called_once_with( + "Unexpected username mismatch during email update. Skipping email update for user %s. " + "User username: %s, Details username: %s", + 'test_user', + 'test_user', + 'different_user' ) - mock_set_attribute.assert_any_call('update_email.username_mismatch', True) - mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', True) - mock_set_attribute.assert_any_call('update_email.details_username', 'different_user') - mock_set_attribute.assert_any_call('update_email.user_username', 'test_user') - mock_set_attribute.assert_any_call('update_email.details_has_email', True) self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=False) - @patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled') - @patch('auth_backends.pipeline.logger') - @patch('auth_backends.pipeline.set_custom_attribute') - def test_username_mismatch_with_update_toggle_disabled(self, mock_set_attribute, mock_logger, mock_toggle): - """ Verify that email is updated when usernames don't match but toggle is disabled. """ - mock_toggle.return_value = False - - old_email = self.user.email - updated_email = 'updated@example.com' - - update_email(self.strategy, {'email': updated_email, 'username': 'different_user'}, user=self.user) - - updated_user = User.objects.get(pk=self.user.pk) - self.assertEqual(updated_user.email, updated_email) - self.assertNotEqual(updated_user.email, old_email) - - mock_logger.warning.assert_called_once() - - mock_set_attribute.assert_any_call('update_email.username_mismatch', True) - mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False) - mock_set_attribute.assert_any_call('update_email.details_username', 'different_user') - mock_set_attribute.assert_any_call('update_email.user_username', 'test_user') - mock_set_attribute.assert_any_call('update_email.details_has_email', True) - self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=True) - def assert_attribute_was_set(self, mock_set_attribute, attribute_name, should_exist=True): """ Assert that a specific attribute was or was not set via set_custom_attribute.