Skip to content

Commit 3444417

Browse files
Akanshu-2uAkanshu-2u
authored
fix:bug in third party authentication email and name update (#382)
Fix: * verify email_updated is not set when email is not updated in update_email tests * improve assertions for email_updated attribute in update_email tests
1 parent b92924f commit 3444417

File tree

6 files changed

+255
-21
lines changed

6 files changed

+255
-21
lines changed

‎CHANGELOG.rst‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,16 @@ Change Log
1212
Unreleased
1313
----------
1414

15-
*
15+
16+
[4.6.0] - 2025-06-18
17+
--------------------
18+
19+
Changed
20+
~~~~~~~
21+
22+
* Improved test coverage by replacing MagicMock with real load_strategy() implementation.
23+
* Fixed email update handling in authentication pipeline.
24+
* Added logging for email updates.
1625

1726
Added
1827
~~~~~~~

‎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.5.0' # pragma: no cover
6+
__version__ = '4.6.0' # pragma: no cover

‎auth_backends/pipeline.py‎

Lines changed: 72 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,27 @@
11
"""Python-social-auth pipeline functions.
22
3-
For more info visit http://python-social-auth.readthedocs.org/en/latest/pipeline.html.
3+
For more info visit https://python-social-auth.readthedocs.io/en/latest/pipeline.html.
44
"""
5+
import logging
56
from django.contrib.auth import get_user_model
7+
from edx_toggles.toggles import SettingToggle
8+
from edx_django_utils.monitoring import set_custom_attribute
69

7-
10+
logger = logging.getLogger(__name__)
811
User = get_user_model()
912

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+
1025

1126
# pylint: disable=unused-argument
1227
# The function parameters must be named exactly as they are below.
@@ -33,9 +48,63 @@ def get_user_if_exists(strategy, details, user=None, *args, **kwargs): # pylint
3348

3449
def update_email(strategy, details, user=None, *args, **kwargs): # pylint: disable=keyword-arg-before-vararg
3550
"""Update the user's email address using data from provider."""
51+
3652
if user:
37-
email = details.get('email')
53+
# Get usernames for comparison, using defensive coding
54+
details_username = details.get('username')
55+
user_username = getattr(user, 'username', None)
56+
# Check if usernames don't match
57+
username_mismatch = details_username != user_username
58+
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())
3869

70+
if username_mismatch:
71+
# Log warning and set additional custom attributes for mismatches
72+
logger.warning(
73+
"Username mismatch during email update. User username: %s, Details username: %s",
74+
user_username,
75+
details_username
76+
)
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
100+
101+
# Proceed with email update only if usernames match or toggle is disabled
102+
email = details.get('email')
39103
if email and user.email != email:
40104
user.email = email
41105
strategy.storage.user.changed(user)
106+
107+
# .. custom_attribute_name: update_email.email_updated
108+
# .. custom_attribute_description: Indicates that the user's email was
109+
# actually updated during this pipeline execution.
110+
set_custom_attribute('update_email.email_updated', True)

‎auth_backends/tests/test_pipeline.py‎

Lines changed: 115 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
""" Tests for pipelines. """
22

3+
from unittest.mock import patch
34
from django.contrib.auth import get_user_model
45
from django.test import TestCase
56
from social_django.utils import load_strategy
@@ -40,20 +41,124 @@ class UpdateEmailPipelineTests(TestCase):
4041

4142
def setUp(self):
4243
super().setUp()
43-
self.user = User.objects.create()
44+
self.user = User.objects.create(username='test_user')
45+
self.strategy = load_strategy()
4446

45-
def test_update_email(self):
46-
""" Verify that user email is updated upon changing email. """
47+
@patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled')
48+
@patch('auth_backends.pipeline.set_custom_attribute')
49+
def test_update_email(self, mock_set_attribute, mock_toggle):
50+
""" Verify that user email is updated upon changing email when usernames match. """
51+
mock_toggle.return_value = False
4752
updated_email = '[email protected]'
4853
self.assertNotEqual(self.user.email, updated_email)
4954

50-
update_email(load_strategy(), {'email': updated_email}, user=self.user)
51-
self.user = User.objects.get(username=self.user.username)
52-
self.assertEqual(self.user.email, updated_email)
55+
initial_email = self.user.email
5356

54-
def test_update_email_with_none(self):
57+
update_email(self.strategy, {'email': updated_email, 'username': 'test_user'}, user=self.user)
58+
59+
updated_user = User.objects.get(pk=self.user.pk)
60+
self.assertEqual(updated_user.email, updated_email)
61+
self.assertNotEqual(updated_user.email, initial_email)
62+
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)
65+
self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=True)
66+
67+
@patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled')
68+
@patch('auth_backends.pipeline.set_custom_attribute')
69+
def test_update_email_with_none(self, mock_set_attribute, mock_toggle):
5570
""" Verify that user email is not updated if email value is None. """
71+
mock_toggle.return_value = False
5672
old_email = self.user.email
57-
update_email(load_strategy(), {'email': None}, user=self.user)
58-
self.user = User.objects.get(username=self.user.username)
59-
self.assertEqual(self.user.email, old_email)
73+
74+
update_email(self.strategy, {'email': None, 'username': 'test_user'}, user=self.user)
75+
76+
updated_user = User.objects.get(pk=self.user.pk)
77+
self.assertEqual(updated_user.email, old_email)
78+
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)
81+
self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=False)
82+
83+
@patch('auth_backends.pipeline.SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH.is_enabled')
84+
@patch('auth_backends.pipeline.logger')
85+
@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+
90+
old_email = self.user.email
91+
updated_email = '[email protected]'
92+
93+
update_email(self.strategy, {'email': updated_email, 'username': 'different_user'}, user=self.user)
94+
95+
updated_user = User.objects.get(pk=self.user.pk)
96+
self.assertEqual(updated_user.email, old_email)
97+
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'
107+
)
108+
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)
114+
self.assert_attribute_was_set(mock_set_attribute, 'update_email.email_updated', should_exist=False)
115+
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 = '[email protected]'
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+
141+
def assert_attribute_was_set(self, mock_set_attribute, attribute_name, should_exist=True):
142+
"""
143+
Assert that a specific attribute was or was not set via set_custom_attribute.
144+
145+
Args:
146+
mock_set_attribute: The mocked set_custom_attribute function
147+
attribute_name: The name of the attribute to check
148+
should_exist: If True, assert the attribute was set; if False, assert it wasn't
149+
"""
150+
matching_calls = [
151+
call for call in mock_set_attribute.call_args_list
152+
if call[0][0] == attribute_name
153+
]
154+
155+
if should_exist:
156+
self.assertGreater(
157+
len(matching_calls), 0,
158+
f"Expected '{attribute_name}' to be set, but it wasn't"
159+
)
160+
else:
161+
self.assertEqual(
162+
len(matching_calls), 0,
163+
f"Expected '{attribute_name}' not to be set, but it was: {matching_calls}"
164+
)

‎requirements/base.in‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@
22
-c constraints.txt
33

44
Django
5+
edx-django-utils
6+
edx-toggles
57
pyjwt[crypto]>=2.1.0 # depends on newer jwt.decode and jwt.encode functionality
68
six
79
social-auth-app-django

‎requirements/base.txt‎

Lines changed: 55 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,39 +6,79 @@
66
#
77
asgiref==3.8.1
88
# via django
9-
certifi==2025.1.31
9+
certifi==2025.4.26
1010
# via requests
1111
cffi==1.17.1
12-
# via cryptography
13-
charset-normalizer==3.4.1
12+
# via
13+
# cryptography
14+
# pynacl
15+
charset-normalizer==3.4.2
1416
# via requests
15-
cryptography==44.0.2
17+
click==8.2.1
18+
# via
19+
# code-annotations
20+
# edx-django-utils
21+
code-annotations==2.3.0
22+
# via edx-toggles
23+
cryptography==45.0.3
1624
# via
1725
# pyjwt
1826
# social-auth-core
1927
defusedxml==0.7.1
2028
# via
2129
# python3-openid
2230
# social-auth-core
23-
django==4.2.20
31+
django==4.2.22
2432
# via
2533
# -c requirements/common_constraints.txt
2634
# -r requirements/base.in
35+
# django-crum
36+
# django-waffle
37+
# edx-django-utils
38+
# edx-toggles
2739
# social-auth-app-django
40+
django-crum==0.7.9
41+
# via
42+
# edx-django-utils
43+
# edx-toggles
44+
django-waffle==4.2.0
45+
# via
46+
# edx-django-utils
47+
# edx-toggles
48+
edx-django-utils==8.0.0
49+
# via
50+
# -r requirements/base.in
51+
# edx-toggles
52+
edx-toggles==5.3.0
53+
# via -r requirements/base.in
2854
idna==3.10
2955
# via requests
56+
jinja2==3.1.6
57+
# via code-annotations
58+
markupsafe==3.0.2
59+
# via jinja2
3060
oauthlib==3.2.2
3161
# via
3262
# requests-oauthlib
3363
# social-auth-core
64+
pbr==6.1.1
65+
# via stevedore
66+
psutil==7.0.0
67+
# via edx-django-utils
3468
pycparser==2.22
3569
# via cffi
3670
pyjwt[crypto]==2.10.1
3771
# via
3872
# -r requirements/base.in
3973
# social-auth-core
74+
pynacl==1.5.0
75+
# via edx-django-utils
76+
python-slugify==8.0.4
77+
# via code-annotations
4078
python3-openid==3.2.0
4179
# via social-auth-core
80+
pyyaml==6.0.2
81+
# via code-annotations
4282
requests==2.32.3
4383
# via
4484
# requests-oauthlib
@@ -49,13 +89,22 @@ six==1.17.0
4989
# via -r requirements/base.in
5090
social-auth-app-django==5.4.3
5191
# via -r requirements/base.in
52-
social-auth-core==4.5.6
92+
social-auth-core==4.6.1
5393
# via
5494
# -r requirements/base.in
5595
# social-auth-app-django
5696
sqlparse==0.5.3
5797
# via django
98+
stevedore==5.4.1
99+
# via
100+
# code-annotations
101+
# edx-django-utils
102+
text-unidecode==1.3
103+
# via python-slugify
58104
urllib3==2.2.3
59105
# via
60106
# -c requirements/common_constraints.txt
61107
# requests
108+
109+
# The following packages are considered to be unsafe in a requirements file:
110+
# setuptools

0 commit comments

Comments
 (0)