diff --git a/.github/dependabot.yml b/.github/dependabot.yml index d0fde72a..f84d3069 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -6,4 +6,4 @@ updates: schedule: interval: "weekly" reviewers: - - "openedx/arbi-bom" + - "openedx/orbi-bom" diff --git a/.github/workflows/upgrade-python-requirements.yml b/.github/workflows/upgrade-python-requirements.yml index b63c0530..d39b2ebb 100644 --- a/.github/workflows/upgrade-python-requirements.yml +++ b/.github/workflows/upgrade-python-requirements.yml @@ -13,8 +13,8 @@ jobs: call-upgrade-python-requirements-workflow: with: branch: ${{ github.event.inputs.branch }} - team_reviewers: "arbi-bom" - email_address: arbi-bom@edx.org + team_reviewers: "orbi-bom" + email_address: orbi-bom@edx.org send_success_notification: false secrets: requirements_bot_github_token: ${{ secrets.REQUIREMENTS_BOT_GITHUB_TOKEN }} diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 741f59fa..76822ae0 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,18 @@ Unreleased ---------- +[4.6.1] - 2025-08-26 +-------------------- + +Fixed +~~~~~ + +* 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. + * Enhanced get_user_if_exists function with monitoring capabilities for debugging authentication conflicts. + [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/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/pipeline.py b/auth_backends/pipeline.py index d92259e6..b402ae84 100644 --- a/auth_backends/pipeline.py +++ b/auth_backends/pipeline.py @@ -22,27 +22,67 @@ # .. toggle_target_removal_date: 2025-08-18 SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH = SettingToggle("SKIP_UPDATE_EMAIL_ON_USERNAME_MISMATCH", default=False) +# .. toggle_name: IGNORE_LOGGED_IN_USER_ON_MISMATCH +# .. toggle_implementation: SettingToggle +# .. 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-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.""" + """ + 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: - return {'is_new': False} + # 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, False if missing. + 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 + # .. 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('get_user_if_exists.username_mismatch', username_mismatch) + + 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.", + details_username, user_username + ) + else: + return {'is_new': False} + try: username = details.get('username') - # Return the user if it exists return { 'is_new': False, 'user': User.objects.get(username=username) } except User.DoesNotExist: - # Fall to the default return value pass - # Nothing to return since we don't have a user return {} 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'): """ diff --git a/auth_backends/tests/test_pipeline.py b/auth_backends/tests/test_pipeline.py index 060dcb30..d9d0cfb2 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 +from unittest.mock import patch, call +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,30 +11,119 @@ User = get_user_model() +@ddt.ddt 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() - self.username = 'edx' - self.details = {'username': self.username} - - def test_no_user_exists(self): - """ Verify an empty dict is returned if no user exists. """ - actual = get_user_if_exists(None, self.details) - self.assertDictEqual(actual, {}) - - def test_existing_user(self): - """ Verify a dict with the user and extra details is returned if the user exists. """ - 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 only the details are returned if a user is passed to the function. """ - user = User.objects.create(username=self.username) - actual = get_user_if_exists(None, self.details, user=user) - self.assertDictEqual(actual, {'is_new': False}) + 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.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): + """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_for_non_existing_user) + expected = {} + self.assertDictEqual(actual, expected) + + @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 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.existing_user + actual = get_user_if_exists(None, self.details_for_existing_user, user=None) + + 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() + + @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 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.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() + + @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_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): + 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) + + if toggle_enabled: + 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', + '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.has_details_username', True), + call('get_user_if_exists.username_mismatch', True) + ], any_order=True) + + @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_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): + existing_user = self.existing_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.", + 'non_existing_user', + '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.has_details_username', True), + call('get_user_if_exists.username_mismatch', True) + ], any_order=True) class UpdateEmailPipelineTests(TestCase): diff --git a/requirements/test.in b/requirements/test.in index 210ca42b..5fb7bc70 100644 --- a/requirements/test.in +++ b/requirements/test.in @@ -4,12 +4,15 @@ -r base.txt # Core dependencies coverage +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 tox +typing_extensions # required by ddt unittest2 -edx-django-release-util # Contains the reserved keyword check +edx-django-release-util # Contains the reserved keyword check diff --git a/requirements/test.txt b/requirements/test.txt index 6add5676..ee46bd39 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,37 +168,48 @@ 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 + # 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 @@ -167,7 +219,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 +228,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 @@ -187,14 +244,16 @@ 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.2.3 +urllib3==2.5.0 # via - # -c requirements/common_constraints.txt # -r requirements/base.txt # requests -virtualenv==20.30.0 + # responses +virtualenv==20.32.0 # via tox # The following packages are considered to be unsafe in a requirements file: