Skip to content

Commit f8e27d9

Browse files
Akanshu-2uAkanshu-2u
authored
fix: cleanup of toggle and few custom attributes (#405)
* fix: cleanup of toggle and few custom attributes * fix: updated version and added in changelog
1 parent fb50db2 commit f8e27d9

6 files changed

Lines changed: 49 additions & 182 deletions

File tree

‎CHANGELOG.rst‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,14 @@ Unreleased
1313
----------
1414

1515

16+
[4.6.2] - 2025-10-16
17+
--------------------
18+
19+
Changed
20+
~~~~~~~
21+
22+
* Removed temporary rollout toggles and simplified custom attribute tracking after successful OAuth session cleanup rollout.
23+
1624
[4.6.1] - 2025-09-23
1725
--------------------
1826

‎auth_backends/__init__.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,4 @@
33
These package is designed to be used primarily with Open edX Django projects, but should be compatible with non-edX
44
projects as well.
55
"""
6-
__version__ = '4.6.1' # pragma: no cover
6+
__version__ = '4.6.2' # pragma: no cover

‎auth_backends/backends.py‎

Lines changed: 2 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -7,23 +7,10 @@
77
from django.contrib.auth import logout
88
from django.dispatch import Signal
99
from social_core.backends.oauth import BaseOAuth2
10-
from edx_toggles.toggles import SettingToggle
1110
from edx_django_utils.monitoring import set_custom_attribute
1211

1312
logger = logging.getLogger(__name__)
1413

15-
# .. toggle_name: ENABLE_OAUTH_SESSION_CLEANUP
16-
# .. toggle_implementation: SettingToggle
17-
# .. toggle_default: False
18-
# .. toggle_description: Controls whether to perform session cleanup during OAuth start.
19-
# When enabled (True), existing user sessions are cleared before OAuth authentication
20-
# to prevent user association conflicts. When disabled (False), session cleanup is skipped.
21-
# This toggle allows for gradual rollout and quick rollback if issues arise.
22-
# .. toggle_use_cases: temporary
23-
# .. toggle_creation_date: 2025-09-25
24-
# .. toggle_target_removal_date: 2025-11-25
25-
ENABLE_OAUTH_SESSION_CLEANUP = SettingToggle("ENABLE_OAUTH_SESSION_CLEANUP", default=False)
26-
2714
PROFILE_CLAIMS_TO_DETAILS_KEY_MAP = {
2815
'preferred_username': 'username',
2916
'email': 'email',
@@ -88,20 +75,10 @@ def logout_url(self):
8875
return self.end_session_url()
8976

9077
def start(self):
91-
"""Initialize OAuth authentication with optional session cleanup."""
92-
93-
# .. custom_attribute_name: session_cleanup.toggle_enabled
94-
# .. custom_attribute_description: Tracks whether the ENABLE_OAUTH_SESSION_CLEANUP
95-
# toggle is enabled during OAuth start.
96-
set_custom_attribute('session_cleanup.toggle_enabled', ENABLE_OAUTH_SESSION_CLEANUP.is_enabled())
78+
"""Initialize OAuth authentication with session cleanup."""
9779

9880
request = self.strategy.request if hasattr(self.strategy, 'request') else None
9981

100-
# .. custom_attribute_name: session_cleanup.has_request
101-
# .. custom_attribute_description: Tracks whether a request object is available
102-
# during OAuth start. True if request exists, False if missing.
103-
set_custom_attribute('session_cleanup.has_request', request is not None)
104-
10582
user_authenticated = (
10683
request is not None and
10784
hasattr(request, 'user') and
@@ -113,28 +90,16 @@ def start(self):
11390
# before session cleanup. True if user was logged in, False otherwise.
11491
set_custom_attribute('session_cleanup.logout_required', user_authenticated)
11592

116-
if user_authenticated and ENABLE_OAUTH_SESSION_CLEANUP.is_enabled():
93+
if user_authenticated:
11794
existing_username = getattr(request.user, 'username', 'unknown')
11895

119-
# .. custom_attribute_name: session_cleanup.logged_out_username
120-
# .. custom_attribute_description: Records the username that was logged out
121-
# during session cleanup for tracking and debugging purposes.
122-
set_custom_attribute('session_cleanup.logged_out_username', existing_username)
123-
12496
logger.info(
12597
"OAuth start: Performing session cleanup for user '%s'",
12698
existing_username
12799
)
128100

129101
logout(request)
130102

131-
# .. custom_attribute_name: session_cleanup.logout_performed
132-
# .. custom_attribute_description: Indicates that session cleanup was
133-
# actually performed during OAuth start.
134-
set_custom_attribute('session_cleanup.logout_performed', True)
135-
else:
136-
set_custom_attribute('session_cleanup.logout_performed', False)
137-
138103
return super().start()
139104

140105
def authorization_url(self):

‎auth_backends/pipeline.py‎

Lines changed: 5 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -4,24 +4,11 @@
44
"""
55
import logging
66
from django.contrib.auth import get_user_model
7-
from edx_toggles.toggles import SettingToggle
87
from edx_django_utils.monitoring import set_custom_attribute
98

109
logger = logging.getLogger(__name__)
1110
User = get_user_model()
1211

13-
# .. toggle_name: SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH
14-
# .. toggle_implementation: SettingToggle
15-
# .. toggle_default: False
16-
# .. toggle_description: Determines whether to block email updates when usernames don't match.
17-
# When enabled (True), email updates will be blocked when the username in social auth details
18-
# doesn't match the user's username. When disabled (False), email updates will proceed regardless
19-
# of username mismatches. This will be used for a temporary rollout.
20-
# .. toggle_use_cases: temporary
21-
# .. toggle_creation_date: 2025-06-18
22-
# .. toggle_target_removal_date: 2025-08-18
23-
SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH = SettingToggle("SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH", default=False)
24-
2512

2613
# pylint: disable=unused-argument
2714
# 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
5643
# Check if usernames don't match
5744
username_mismatch = details_username != user_username
5845

59-
# .. custom_attribute_name: update_email.username_mismatch
60-
# .. custom_attribute_description: Tracks whether there's a mismatch between
61-
# the username in the social details and the user's actual username.
62-
# True if usernames don't match, False if they match.
63-
set_custom_attribute('update_email.username_mismatch', username_mismatch)
64-
65-
# .. custom_attribute_name: update_email.rollout_toggle_enabled
66-
# .. custom_attribute_description: Tracks whether the SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH
67-
# toggle is enabled during this pipeline execution.
68-
set_custom_attribute('update_email.rollout_toggle_enabled', SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled())
69-
7046
if username_mismatch:
71-
# Log warning and set additional custom attributes for mismatches
7247
logger.warning(
73-
"Username mismatch during email update. User username: %s, Details username: %s",
48+
"Unexpected username mismatch during email update. Skipping email update for user %s. "
49+
"User username: %s, Details username: %s",
50+
user_username,
7451
user_username,
7552
details_username
7653
)
77-
# .. custom_attribute_name: update_email.details_username
78-
# .. custom_attribute_description: Records the username provided in the
79-
# social details when a mismatch occurs with the user's username.
80-
set_custom_attribute('update_email.details_username', details_username)
81-
82-
# .. custom_attribute_name: update_email.user_username
83-
# .. custom_attribute_description: Records the actual username of the user
84-
# when a mismatch occurs with the social details username.
85-
set_custom_attribute('update_email.user_username', user_username)
86-
87-
# .. custom_attribute_name: update_email.details_has_email
88-
# .. custom_attribute_description: Records whether the details contain an email
89-
# when a username mismatch occurs, to identify potential edge cases.
90-
set_custom_attribute('update_email.details_has_email', bool(details.get('email')))
91-
92-
# Only exit if the toggle is enabled
93-
if SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled():
94-
logger.warning(
95-
"Skipping email update for user %s due to username mismatch and "
96-
"SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH toggle enabled",
97-
user_username
98-
)
99-
return # Exit without updating email
54+
return # Exit without updating email
10055

101-
# Proceed with email update only if usernames match or toggle is disabled
56+
# Proceed with email update only if usernames match
10257
email = details.get('email')
10358
if email and user.email != email:
10459
user.email = email

‎auth_backends/tests/test_backends.py‎

Lines changed: 23 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
import datetime
33
import json
44
from calendar import timegm
5-
from unittest.mock import patch, call
5+
from unittest.mock import patch
66

77
import ddt
88
import jwt
@@ -14,7 +14,6 @@
1414
from django.contrib.sessions.middleware import SessionMiddleware
1515
from django.core.cache import cache
1616
from django.test import RequestFactory
17-
from django.test.utils import override_settings
1817
from social_core.tests.backends.oauth import OAuth2Test
1918

2019
User = get_user_model()
@@ -127,54 +126,38 @@ def test_login(self):
127126
self.do_login()
128127

129128
@pytest.mark.django_db
130-
@ddt.data(True, False) # Test session cleanup with both toggle enabled and disabled
129+
@ddt.data(True, False) # Test with and without authenticated user
131130
@patch('auth_backends.backends.set_custom_attribute')
132131
@patch('auth_backends.backends.logger')
133-
def test_start_with_session_cleanup(self, toggle_enabled, mock_logger, mock_set_attr):
134-
"""Test start method for session cleanup of existing user with toggle variation."""
135-
with override_settings(ENABLE_OAUTH_SESSION_CLEANUP=toggle_enabled):
136-
existing_user = User.objects.create_user(username='existing_user', email='existing@example.com')
132+
def test_start_with_session_cleanup(self, user_authenticated, mock_logger, mock_set_attr):
133+
"""Test start method for session cleanup with and without authenticated user."""
134+
request = RequestFactory().get('/auth/login/edx-oauth2/')
137135

138-
request = RequestFactory().get('/auth/login/edx-oauth2/')
136+
if user_authenticated:
137+
existing_user = User.objects.create_user(username='existing_user', email='existing@example.com')
139138
request.user = existing_user
140139

141-
middleware = SessionMiddleware(lambda req: None)
142-
middleware.process_request(request)
143-
request.session.save()
144-
145-
initial_session_key = request.session.session_key
146-
147-
self.backend.strategy.request = request
148-
149-
self.do_start()
150-
151-
if toggle_enabled:
152-
self.assertNotEqual(request.session.session_key, initial_session_key)
153-
154-
self.assertTrue(request.user.is_anonymous)
140+
middleware = SessionMiddleware(lambda req: None)
141+
middleware.process_request(request)
142+
request.session.save()
155143

156-
mock_set_attr.assert_has_calls([
157-
call('session_cleanup.toggle_enabled', True),
158-
call('session_cleanup.logout_performed', True),
159-
call('session_cleanup.logged_out_username', 'existing_user')
160-
], any_order=True)
144+
initial_session_key = request.session.session_key
161145

162-
mock_logger.info.assert_called_with(
163-
"OAuth start: Performing session cleanup for user '%s'",
164-
'existing_user'
165-
)
166-
else:
167-
self.assertEqual(request.session.session_key, initial_session_key)
146+
self.backend.strategy.request = request
168147

169-
self.assertEqual(request.user, existing_user)
170-
self.assertFalse(request.user.is_anonymous)
148+
self.do_start()
171149

172-
mock_set_attr.assert_has_calls([
173-
call('session_cleanup.toggle_enabled', False),
174-
call('session_cleanup.logout_performed', False)
175-
], any_order=True)
150+
mock_set_attr.assert_called_once_with('session_cleanup.logout_required', user_authenticated)
176151

177-
mock_logger.info.assert_not_called()
152+
if user_authenticated:
153+
self.assertNotEqual(request.session.session_key, initial_session_key)
154+
self.assertTrue(request.user.is_anonymous)
155+
mock_logger.info.assert_called_with(
156+
"OAuth start: Performing session cleanup for user '%s'",
157+
'existing_user'
158+
)
159+
else:
160+
mock_logger.info.assert_not_called()
178161

179162
def test_partial_pipeline(self):
180163
self.do_partial_pipeline()

‎auth_backends/tests/test_pipeline.py‎

Lines changed: 10 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,9 @@ def setUp(self):
4444
self.user = User.objects.create(username='test_user')
4545
self.strategy = load_strategy()
4646

47-
@patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled')
4847
@patch('auth_backends.pipeline.set_custom_attribute')
49-
def test_update_email(self, mock_set_attribute, mock_toggle):
48+
def test_update_email(self, mock_set_attribute):
5049
""" Verify that user email is updated upon changing email when usernames match. """
51-
mock_toggle.return_value = False
5250
updated_email = 'updated@example.com'
5351
self.assertNotEqual(self.user.email, updated_email)
5452

@@ -60,33 +58,24 @@ def test_update_email(self, mock_set_attribute, mock_toggle):
6058
self.assertEqual(updated_user.email, updated_email)
6159
self.assertNotEqual(updated_user.email, initial_email)
6260

63-
mock_set_attribute.assert_any_call('update_email.username_mismatch', False)
64-
mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False)
6561
self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=True)
6662

67-
@patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled')
6863
@patch('auth_backends.pipeline.set_custom_attribute')
69-
def test_update_email_with_none(self, mock_set_attribute, mock_toggle):
64+
def test_update_email_with_none(self, mock_set_attribute):
7065
""" Verify that user email is not updated if email value is None. """
71-
mock_toggle.return_value = False
7266
old_email = self.user.email
7367

7468
update_email(self.strategy, {'email': None, 'username': 'test_user'}, user=self.user)
7569

7670
updated_user = User.objects.get(pk=self.user.pk)
7771
self.assertEqual(updated_user.email, old_email)
7872

79-
mock_set_attribute.assert_any_call('update_email.username_mismatch', False)
80-
mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False)
8173
self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=False)
8274

83-
@patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled')
8475
@patch('auth_backends.pipeline.logger')
8576
@patch('auth_backends.pipeline.set_custom_attribute')
86-
def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mock_logger, mock_toggle):
87-
""" Verify that email is not updated when usernames don't match and toggle is enabled. """
88-
mock_toggle.return_value = True
89-
77+
def test_username_mismatch_no_update(self, mock_set_attribute, mock_logger):
78+
""" Verify that email is not updated when usernames don't match. """
9079
old_email = self.user.email
9180
updated_email = 'updated@example.com'
9281

@@ -95,49 +84,16 @@ def test_username_mismatch_no_update_toggle_enabled(self, mock_set_attribute, mo
9584
updated_user = User.objects.get(pk=self.user.pk)
9685
self.assertEqual(updated_user.email, old_email)
9786

98-
self.assertEqual(mock_logger.warning.call_count, 2)
99-
mock_logger.warning.assert_any_call(
100-
"Username mismatch during email update. User username: %s, Details username: %s",
101-
'test_user', 'different_user'
102-
)
103-
mock_logger.warning.assert_any_call(
104-
"Skipping email update for user %s due to username mismatch and "
105-
"SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH toggle enabled",
106-
'test_user'
87+
mock_logger.warning.assert_called_once_with(
88+
"Unexpected username mismatch during email update. Skipping email update for user %s. "
89+
"User username: %s, Details username: %s",
90+
'test_user',
91+
'test_user',
92+
'different_user'
10793
)
10894

109-
mock_set_attribute.assert_any_call('update_email.username_mismatch', True)
110-
mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', True)
111-
mock_set_attribute.assert_any_call('update_email.details_username', 'different_user')
112-
mock_set_attribute.assert_any_call('update_email.user_username', 'test_user')
113-
mock_set_attribute.assert_any_call('update_email.details_has_email', True)
11495
self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=False)
11596

116-
@patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled')
117-
@patch('auth_backends.pipeline.logger')
118-
@patch('auth_backends.pipeline.set_custom_attribute')
119-
def test_username_mismatch_with_update_toggle_disabled(self, mock_set_attribute, mock_logger, mock_toggle):
120-
""" Verify that email is updated when usernames don't match but toggle is disabled. """
121-
mock_toggle.return_value = False
122-
123-
old_email = self.user.email
124-
updated_email = 'updated@example.com'
125-
126-
update_email(self.strategy, {'email': updated_email, 'username': 'different_user'}, user=self.user)
127-
128-
updated_user = User.objects.get(pk=self.user.pk)
129-
self.assertEqual(updated_user.email, updated_email)
130-
self.assertNotEqual(updated_user.email, old_email)
131-
132-
mock_logger.warning.assert_called_once()
133-
134-
mock_set_attribute.assert_any_call('update_email.username_mismatch', True)
135-
mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False)
136-
mock_set_attribute.assert_any_call('update_email.details_username', 'different_user')
137-
mock_set_attribute.assert_any_call('update_email.user_username', 'test_user')
138-
mock_set_attribute.assert_any_call('update_email.details_has_email', True)
139-
self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=True)
140-
14197
def assert_attribute_was_set(self, mock_set_attribute, attribute_name, should_exist=True):
14298
"""
14399
Assert that a specific attribute was or was not set via set_custom_attribute.

0 commit comments

Comments
 (0)