fix:bug in third party authentication email and name update #382

Akanshu-2u · 2025-06-02T14:20:52Z

Description:

This PR enhances the update_email pipeline function with security verification that ensures email updates only occur when usernames match between the authentication provider and the existing user account. It adds detailed logging and monitoring attributes to track username mismatches, helping identify potential security issues or integration problems in the authentication flow.

Added Username Verification:
- Now validates that the username from the authentication provider matches the user's username before updating email.
- Uses defensive coding with [details.get('username')] and [getattr(user, 'username', None)].

Added Monitoring and Logging:

Sets a custom attribute [update_email.username_mismatch] for all cases to track mismatches.

For mismatches only:

 - Logs a detailed warning with both usernames.

 - Sets additional custom attributes [update_email.details_username] and [update_email.user_username].
 
 - Stops execution before email update occurs.

Security Improvement:
- Prevents potential account takeover by ensuring emails are only updated when the authentication flow correctly identifies
  the user.
- Provides visibility into authentication integration issues through the added monitoring.

Unit-Tests

Replaced real strategy objects with [MagicMock] for better test isolation and reliability.
Added realistic behavior to mocks with [side_effect] to properly test database interactions.
Added comprehensive test coverage for success paths, edge cases, and security scenarios.

Created specific test for username mismatch scenario to verify security controls.

Testing Performed

  - Email update succeeds with matching usernames
  - Email update skipped for null email values
  - Email update blocked when usernames don't match

JIRA:
(https://2u-internal.atlassian.net/browse/ARCHBOM-2181)

Reviewers:
@robrap

robrap

Thank you.

Please read https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0051-bp-conventional-commits.html for how to format your commit messages. You can use "fix: ...", or "fixup! ..." as you add new commits to the PR (that can be squashed at the end).
You have "... and name update" in the PR title. Is there supposed to be something about this? Also, do we need to look into whether fullname is updated in another step that might have the same issue?
Are there unit tests for update_email you can enhance, or add?

robrap · 2025-06-02T19:38:14Z

requirements/ci.txt

    #   -r requirements/ci.in
 virtualenv==20.30.0
    # via tox
+edx-django-utils==8.0.0


I appreciate you looking into CI failures. Definitely worth looking into before (or in parallel) with a review, as long as you let the reviewer know that you are aware of the failures and are looking into it.

See https://open-edx-proposals.readthedocs.io/en/latest/best-practices/oep-0018-bp-python-dependencies.html for some help. It is a decision doc, and not a how-to, so it isn't as brief as it could be.

This should be moved to base.in without a version.

There is a github action for upgrading: https://github.com/openedx/auth-backends/actions/workflows/upgrade-python-requirements.yml. You might not be able to run this. Do you have it on your fork, and can you run there? If not, you can run make upgrade from a container (which doesn't always work, because it may not match Python exactly, or other small differences).

Leaving this unresolved until part 3 is handled. Thanks.

This can be marked resolved.

@Akanshu-2u:

Do you see a "Resolve" button on this conversation in github, and are you able to mark as resolved?

Are you able to give me write access on your private fork, and does that give me the capability to resolve conversations here?
Thank you.

Yes, I am able to see "Resolve" button and can mark it as resolved.

Ok. Thank you.

@Akanshu-2u: You do not need to do anything on this PR, but when I had written all of the comments like "This can be marked resolved.", it would have helped me if you had actually marked that am as resolved. I thought you couldn't, but I was wrong. We can try this on the new PR. Or, you can give me write access on your fork, and maybe I can resolve myself. Thanks.

robrap · 2025-06-02T21:33:21Z

auth_backends/pipeline.py

 from django.contrib.auth import get_user_model
+import logging
+from edx_django_utils.monitoring import set_custom_attribute


I'm guessing you might have sorting issue here. Not sure if isort is running. I'm not sure if what I added below is correct, but it may be closer.

Suggested change

from django.contrib.auth import get_user_model

import logging

from edx_django_utils.monitoring import set_custom_attribute

import logging

from django.contrib.auth import get_user_model

from edx_django_utils.monitoring import set_custom_attribute

This can be marked resolved.

robrap · 2025-06-02T21:35:14Z

auth_backends/pipeline.py

+    
+    Before updating the email, verify that the username in details matches the user's username.
+    If usernames don't match, log a warning and don't update the email.
+    Custom attributes are set to track username mismatches.


I appreciate good comment, but maybe these details could just be left to well commented code below. This is a judgement call. What you have is not wrong. I'd personally go lighter here. The more text, the more comments can drift from code.

This can be marked resolved.

robrap · 2025-06-02T21:39:36Z

auth_backends/pipeline.py

+        user_username = getattr(user, 'username', None)
+
+        # Check if usernames match
+        username_match = details_username and user_username and details_username == user_username


Wouldn't this be the same? You already know these have a value, even if None.

Suggested change

username_match = details_username and user_username and details_username == user_username

username_match = details_username == user_username

This can be marked resolved.

robrap · 2025-06-02T21:40:36Z

auth_backends/pipeline.py

+        username_match = details_username and user_username and details_username == user_username
+
+        # Set custom attribute for all cases
+        set_custom_attribute('update_email.username_mismatch', not username_match)


Please follow openedx/edx-django-utils#328 for annotating/documenting each custom attribute.

This can be marked resolved.

robrap

Thanks for the updates.

robrap · 2025-06-04T20:20:31Z

auth_backends/pipeline.py

@@ -1,10 +1,13 @@
 """Python-social-auth pipeline functions.

 For more info visit http://python-social-auth.readthedocs.org/en/latest/pipeline.html.


I noticed that this link is broken, and should be updated to:

https://python-social-auth.readthedocs.io/en/latest/pipeline.html

This can be marked resolved.

robrap · 2025-06-04T20:24:17Z

auth_backends/pipeline.py

+            )
+            # .. custom_attribute_name: update_email.details_username
+            # .. custom_attribute_description: Records the username provided in the
+            #    authentication details when a mismatch occurs with the user's username.


I got the term "social details" from the doc https://python-social-auth.readthedocs.io/en/latest/pipeline.html I noted elsewhere. I don't want to call this "authentication details", because if the authenticated user (user) is a different user, the authentication details would not be for a different user.

Suggested change

# authentication details when a mismatch occurs with the user's username.

# social details when a mismatch occurs with the user's username.

This can be marked resolved.

robrap · 2025-06-04T20:25:02Z

auth_backends/pipeline.py


+            # .. custom_attribute_name: update_email.user_username
+            # .. custom_attribute_description: Records the actual username of the user
+            #    when a mismatch occurs with the authentication details username.


Again, I want to be careful about using the term "authentication" here.

Suggested change

# when a mismatch occurs with the authentication details username.

# when a mismatch occurs with the social details' username.

This can be marked resolved.

robrap · 2025-06-04T20:34:30Z

auth_backends/tests/test_pipeline.py

+        # Verify email was updated
        self.user = User.objects.get(username=self.user.username)
        self.assertEqual(self.user.email, updated_email)
+        self.strategy.storage.user.changed.assert_called_once_with(self.user)


This assertion on a mock seems unnecessary, since you've already asserted that the email was updated. Can you explain why this is needed?

The assertion "self.strategy.storage.user.changed.assert_called_once_with(self.user)" verifies that our function correctly notifies the strategy when a user is changed.

While the email equality check confirms the outcome (email updated), this assertion verifies the mechanism (strategy notification). This is crucial because:

- It ensures the function fulfills its complete contract with the authentication system. - It prevents refactoring that might bypass the strategy notification while still updating the email. - It maintains consistency with social-auth-core's expected pipeline behavior.

In short, it tests both what happened and how it happened - both essential for complete test coverage.

This can be marked resolved.

robrap · 2025-06-04T20:37:59Z

auth_backends/tests/test_pipeline.py

+        # Verify email was not updated
+        self.user = User.objects.get(username=self.user.username)
+        self.assertEqual(self.user.email, old_email)
+        self.strategy.storage.user.changed.assert_not_called()


Again, this assertion feels a little like overkill, unless you want to be very clear that the email didn't change and change back or something. I'll leave it up to you.

Ok. You can ignore this. I'll go with your earlier explanation. Thanks.

This can be marked resolved.

robrap · 2025-06-04T20:42:41Z

requirements/base.in

 six
 social-auth-app-django
 social-auth-core           # Common auth interfaces
+edx-django-utils


Please alphabetize.

Please set your editor to ensure a blank line at the end of files.

See fix:bug in third party authentication email and name update #382 (comment). You'll still need to run make upgrade from a container to build the *.txt files appropriately. Let me know if this gives you issues.
Thank you.

This can be marked resolved.

robrap · 2025-06-04T20:56:16Z

auth_backends/pipeline.py

+            # .. custom_attribute_description: Records the actual username of the user
+            #    when a mismatch occurs with the authentication details username.
+            set_custom_attribute('update_email.user_username', str(user_username))
+            return  # Exit without updating email


I'd like to add a temporary rollout toggle before this return. It could be a SettingToggle, based on a setting named SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH. See docs:

https://docs.openedx.org/projects/edx-toggles/en/latest/how_to/implement_the_right_toggle_type.html (to implement)

https://docs.openedx.org/projects/edx-toggles/en/latest/how_to/documenting_new_feature_toggles.html (to annotate)

I'm a little nervous about if it is possible to have None for the details' username, but still have a details' email provided. Adding this observability, with the toggle disabled, would allow us to test before enabling the toggle. We could later clean that up.

We might want to add the following to determine if this is ever the case:

set_custom_attribute('update_email.details_has_email', details.get('email') is not None)

If everything looks good from an observability perspective, we can enable the toggle. If things still look good, we can remove the temporary toggle.

Please let me know if you have any questions about this. Thank you.

This can be marked resolved.

robrap · 2025-06-04T21:30:16Z

[request] You'll also want to update the version and changelog to 4.6.0. See example changes needed in this commit.

…gle support

…clean up test cases

…il tests

…CH toggle

robrap · 2025-06-11T20:56:56Z

auth_backends/pipeline.py

+# .. 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.


Suggested change

# of username mismatches.

# of username mismatches. This will be used for a temporary rollout.

This can be marked resolved.

robrap

Thank you.

robrap · 2025-06-11T20:57:26Z

auth_backends/pipeline.py

+# .. toggle_use_cases: temporary
+# .. toggle_creation_date: 2025-06-05
+# .. toggle_target_removal_date: 2026-06-05
+# .. toggle_warnings: This is a temporary toggle for a security enhancement rollout.


Moved and edited in https://github.com/openedx/auth-backends/pull/382/files#r2141085575.

Suggested change

# .. toggle_warnings: This is a temporary toggle for a security enhancement rollout.

This can be marked resolved.

robrap · 2025-06-11T20:58:55Z

auth_backends/pipeline.py

+# .. toggle_creation_date: 2025-06-05
+# .. toggle_target_removal_date: 2026-06-05
+# .. toggle_warnings: This is a temporary toggle for a security enhancement rollout.
+# .. toggle_tickets: ARCHBOM-2181


We can't link to private tickets in public repositories. This PR and its description will be enough.

Suggested change

# .. toggle_tickets: ARCHBOM-2181

This can be marked resolved.

robrap · 2025-06-11T21:01:34Z

auth_backends/pipeline.py

+            # .. custom_attribute_name: update_email.details_username
+            # .. custom_attribute_description: Records the username provided in the
+            #    social details when a mismatch occurs with the user's username.
+            set_custom_attribute('update_email.details_username', str(details_username))


Let the observability tool treat None differently from '' if it wishes.

Suggested change

set_custom_attribute('update_email.details_username', str(details_username))

set_custom_attribute('update_email.details_username', details_username)

This can be marked resolved.

robrap · 2025-06-11T21:01:55Z

auth_backends/pipeline.py

+            # .. custom_attribute_name: update_email.user_username
+            # .. custom_attribute_description: Records the actual username of the user
+            #    when a mismatch occurs with the social details username.
+            set_custom_attribute('update_email.user_username', str(user_username))


Suggested change

set_custom_attribute('update_email.user_username', str(user_username))

set_custom_attribute('update_email.user_username', user_username)

This can be marked resolved.

auth_backends/tests/test_pipeline.py

robrap · 2025-06-11T21:07:44Z

requirements/ci.txt

    #   -r requirements/ci.in
 virtualenv==20.30.0
    # via tox
+edx-django-utils==8.0.0


This can be marked resolved.

auth_backends/__init__.py

robrap · 2025-06-12T00:37:45Z

@Akanshu-2u: You can ignore all comment threads ending with "This can be marked resolved." Thank you.

…update_email pipeline

robrap

This is looking good and is very close, despite the fact that I left a bunch of comments. Thank you!

robrap · 2025-06-16T19:58:14Z

CHANGELOG.rst

 Unreleased
 ----------

 *


Switching this to a blank line:

Suggested change

*

This can be marked resolved.

robrap · 2025-06-16T19:59:11Z

CHANGELOG.rst

 ----------

 *
+[4.6.0] - 2025-06-09


We might update again, but this is closer.

Suggested change

[4.6.0] - 2025-06-09

[4.6.0] - 2025-06-18

This can be marked resolved.

robrap · 2025-06-16T20:00:21Z

auth_backends/pipeline.py

+# .. 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.


Original suggestion had a space.

Suggested change

# of username mismatches.This will be used for a temporary rollout.

# of username mismatches. This will be used for a temporary rollout.

This can be marked resolved.

robrap · 2025-06-16T20:03:03Z

auth_backends/pipeline.py

+# .. toggle_creation_date: 2025-06-05
+# .. toggle_target_removal_date: 2026-06-05


Let's give a shorter window for follow-up. I also updated the date.

Suggested change

# .. toggle_creation_date: 2025-06-05

# .. toggle_target_removal_date: 2026-06-05

# .. toggle_creation_date: 2025-06-18

# .. toggle_target_removal_date: 2025-08-18

This can be marked resolved.

robrap · 2025-06-16T20:46:07Z

auth_backends/pipeline.py

+        details_username = details.get('username')
+        user_username = getattr(user, 'username', None)
+        # Check if usernames match
+        username_match = details_username == user_username


If this were username_mismatch (in place of username_match), you would avoid the not in several places below, which would read more like english, and would be more consistent with the temporary toggle name.

This can be marked resolved.

robrap · 2025-06-16T20:48:09Z

auth_backends/pipeline.py

+            # .. custom_attribute_name: update_email.details_has_email
+            # .. custom_attribute_description: Records whether the details contain an email
+            #    when a username mismatch occurs, to identify potential edge cases.
+            set_custom_attribute('update_email.details_has_email', details.get('email') is not None)


I'm not sure when and if you might get an empty email here, so let's use bool instead. You do not need to add another unit test. This is just in case.

Suggested change

set_custom_attribute('update_email.details_has_email', details.get('email') is not None)

set_custom_attribute('update_email.details_has_email', bool(details.get('email')))

This can be marked resolved.

auth_backends/tests/test_pipeline.py

…ine tests for username mismatch toggle

robrap

Looks great. Let me know if you want to make this final change.

robrap · 2025-06-17T12:34:26Z

auth_backends/tests/test_pipeline.py

-        mock_set_attribute.assert_called_with('update_email.username_mismatch', False)
+        mock_set_attribute.assert_any_call('update_email.username_mismatch', False)
+        mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False)
+        # email_updated should not be called since email wasn't updated


[optional/non-blocking] The mock has an array of calls args. Consider checking that the custom attribute was not called in place of a comment. Consider adding the same check to the True case to show that the code is appropriately checking, because it would be simple to code up a false positive that isn't really checking what you want.

Please leave a comment if you decide to leave as-is.

This can be marked resolved.

…ate_email tests

robrap · 2025-06-17T15:54:50Z

auth_backends/tests/test_pipeline.py

        mock_set_attribute.assert_any_call('update_email.rollout_toggle_enabled', False)
-        # email_updated should not be called since email wasn't updated
+        # Verify email_updated was not set
+        for call_args in mock_set_attribute.call_args_list:


I would use a list comprehension, because then you could easily assert that it is not found, but also assert that it was found.

If you put this in a helper function, like assert_set_attribute_call(found = False), you could use the method in the earlier unit test, and prove that your code would find it if it could.
See example code:

>>> call_args = [('a', 1), ('b', 2)] >>> [x for x in call_args if x[0] == 'a'] [('a', 1)] >>> len([x for x in call_args if x[0] == 'a']) == 0 False >>> len([x for x in call_args if x[0] == 'c']) == 0 True

This can be marked resolved.

robrap · 2025-06-17T16:01:23Z

auth_backends/tests/test_pipeline.py

-        # email_updated should not be called since email wasn't updated
+        # Verify email_updated was not set
+        for call_args in mock_set_attribute.call_args_list:
+            self.assertNotEqual(call_args[0][0], 'update_email.email_updated')


It's unclear if call_args[0][0] is correct or not. As noted in the other column, if you use the same method to assert elsewhere that this call is found, it would make it easier to trust that it not being found is not a false positive.

This can be marked resolved.

…ests

robrap

@openedx/committers-auth-backends: [request] I have reviewed and approved. If you are able to approve, squash and release the library, that would be great. Thank you!

We plan to remove temporary toggles once we've proven this safe.

UPDATE: The PR description (which I can't update) reads:

Prevents potential account takeover by ensuring emails are only updated when the authentication flow correctly identifies the user.
This isn't really true. User records are updated with the wrong email, but since this happens in a service other than the identity provider, there is no account takeover. It does cause invalid data.

idegtiarov

@Akanshu-2u @robrap thank you guys for the great work you did on that PR.

robrap reviewed Jun 2, 2025

View reviewed changes

Akanshu-2u changed the title ~~994 bug in third party authentication email and name update~~ fix: 994 bug in third party authentication email and name update Jun 3, 2025

fix: bug in third party authentication email and name update

68fbf10

Akanshu-2u force-pushed the 994-bug-in-third-party-authentication-email-and-name-update branch from 7e5adfa to 68fbf10 Compare June 3, 2025 09:29

Akanshu-2u changed the title ~~fix: 994 bug in third party authentication email and name update~~ fix:bug in third party authentication email and name update Jun 3, 2025

robrap reviewed Jun 4, 2025

View reviewed changes

Akanshu-2u added 6 commits June 9, 2025 10:16

feat: update version to 4.6.0 and enhance email update logic with tog…

17019a2

…gle support

feat: add edx-toggles to requirements and update dependency comments

55a1470

fix: improve logging message formatting in update_email function and …

b9655b0

…clean up test cases

fix: fixed lint errors in pipeline.py file.

ac6a481

fix: enhance logging verification for username mismatch in update_ema…

49e3b35

…il tests

fix: add target removal date for SKIP_UPDATE_EMAIL_ON_USERNAME_MISMAT…

3be8be1

…CH toggle

robrap reviewed Jun 11, 2025

View reviewed changes

Akanshu-2u added 2 commits June 12, 2025 09:23

fix: update CHANGELOG for version 4.6.0 and improve test coverage in …

933a0dc

…update_email pipeline

fix: remove unnecessary blank line in update_email test case

6435564

robrap reviewed Jun 16, 2025

View reviewed changes

fix: update CHANGELOG for version 4.6.0 and adjust email update pipel…

0ca9058

…ine tests for username mismatch toggle

robrap approved these changes Jun 17, 2025

View reviewed changes

fix: verify email_updated is not set when email is not updated in upd…

9eb3246

…ate_email tests

robrap reviewed Jun 17, 2025

View reviewed changes

fix: improve assertions for email_updated attribute in update_email t…

573ebfb

…ests

robrap approved these changes Jun 18, 2025

View reviewed changes

robrap requested a review from a team June 18, 2025 07:37

robrap mentioned this pull request Jun 18, 2025

fix!: session ID stability while maintaining inactivity timeout openedx/edx-platform#36896

Merged

idegtiarov approved these changes Jun 23, 2025

View reviewed changes

idegtiarov merged commit 3444417 into openedx:master Jun 23, 2025
12 checks passed

Akanshu-2u mentioned this pull request Jul 24, 2025

fix: enhance get_user_if_exists pipeline function with debugging capabilities #390

Closed

Akanshu-2u mentioned this pull request Sep 3, 2025

fix: enhance get_user_if_exists pipeline function with debugging capabilities edx/auth-backends#1

Merged

Akanshu-2u mentioned this pull request Oct 13, 2025

fix: cleanup of toggle and few custom attributes #405

Merged

	username_match = details_username and user_username and details_username == user_username
	username_match = details_username == user_username

		@@ -1,10 +1,13 @@
		"""Python-social-auth pipeline functions.

		For more info visit http://python-social-auth.readthedocs.org/en/latest/pipeline.html.

	# authentication details when a mismatch occurs with the user's username.
	# social details when a mismatch occurs with the user's username.

	# when a mismatch occurs with the authentication details username.
	# when a mismatch occurs with the social details' username.

	# of username mismatches.
	# of username mismatches. This will be used for a temporary rollout.

	set_custom_attribute('update_email.details_username', str(details_username))
	set_custom_attribute('update_email.details_username', details_username)

	set_custom_attribute('update_email.user_username', str(user_username))
	set_custom_attribute('update_email.user_username', user_username)

	# of username mismatches.This will be used for a temporary rollout.
	# of username mismatches. This will be used for a temporary rollout.

		# .. toggle_creation_date: 2025-06-05
		# .. toggle_target_removal_date: 2026-06-05

fix:bug in third party authentication email and name update #382

fix:bug in third party authentication email and name update #382

Uh oh!

Conversation

Akanshu-2u commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

robrap commented Jun 4, 2025

Akanshu-2u commented Jun 2, 2025 •

edited

Loading

robrap Jun 11, 2025 •

edited

Loading