From ffc52aeaef98c98f77f8bfed568af815fe70e648 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Wed, 16 Sep 2026 15:53:40 -0400 Subject: [PATCH] fix: look up request country code without storing it in the session CountryMiddleware wrote country_code and ip_address into the session on every request whose client IP differed from the stored one. For a visitor without a session cookie that is every request, so each response created a new session and set a session cookie, including responses such as course assets that otherwise have no use for a session. Replace the middleware with geoinfo.api.country_code_for_request, which the two readers (block rendering and localized price text) call directly. The lookup is memoized on the request and never touches the session. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019Sz2VhptEzudj4pimSv1bF --- lms/djangoapps/courseware/block_render.py | 3 +- lms/envs/common.py | 1 - .../djangoapps/catalog/tests/test_utils.py | 4 +- openedx/core/djangoapps/catalog/utils.py | 4 +- openedx/core/djangoapps/geoinfo/api.py | 23 ++++ openedx/core/djangoapps/geoinfo/middleware.py | 47 ------- .../core/djangoapps/geoinfo/tests/test_api.py | 77 +++++++++++ .../geoinfo/tests/test_middleware.py | 124 ------------------ xmodule/video_block/video_block.py | 1 - 9 files changed, 106 insertions(+), 178 deletions(-) delete mode 100644 openedx/core/djangoapps/geoinfo/middleware.py create mode 100644 openedx/core/djangoapps/geoinfo/tests/test_api.py delete mode 100644 openedx/core/djangoapps/geoinfo/tests/test_middleware.py diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 8ef637b82bf0..5ca85c576199 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -71,6 +71,7 @@ from openedx.core.djangoapps.credit.services import CreditService from openedx.core.djangoapps.discussions.services import DiscussionConfigService from openedx.core.djangoapps.enrollments.services import EnrollmentsService +from openedx.core.djangoapps.geoinfo.api import country_code_for_request from openedx.core.djangoapps.util.user_utils import SystemUser from openedx.core.djangoapps.video_config.services import VideoConfigService from openedx.core.djangolib.markup import HTML @@ -409,7 +410,7 @@ def get_block_for_descriptor( """ if request: track_function = track_function or make_track_function(request) - user_location = user_location or getattr(request, 'session', {}).get('country_code') + user_location = user_location or country_code_for_request(request) or None request_token = request_token or xblock_request_token(request) if not student_data: diff --git a/lms/envs/common.py b/lms/envs/common.py index dcc51f9ffaad..4e34de151a34 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1254,7 +1254,6 @@ 'openedx.core.djangoapps.cors_csrf.middleware.CorsCSRFMiddleware', 'openedx.core.djangoapps.cors_csrf.middleware.CsrfCrossDomainCookieMiddleware', - 'openedx.core.djangoapps.geoinfo.middleware.CountryMiddleware', 'openedx.core.djangoapps.embargo.middleware.EmbargoMiddleware', # Allows us to use enterprise customer's language as the learner's default language diff --git a/openedx/core/djangoapps/catalog/tests/test_utils.py b/openedx/core/djangoapps/catalog/tests/test_utils.py index eb961951352b..11b9dfdb2865 100644 --- a/openedx/core/djangoapps/catalog/tests/test_utils.py +++ b/openedx/core/djangoapps/catalog/tests/test_utils.py @@ -397,9 +397,9 @@ def test_localized_string(self, mock_get_currency_data): mock_get_currency_data.return_value = currency_data request = RequestFactory().get("/dummy-url") - request.session = {"country_code": "CA"} expected_result = "$20 CAD" - assert get_localized_price_text(10, request) == expected_result + with mock.patch(UTILS_MODULE + ".country_code_for_request", return_value="CA"): + assert get_localized_price_text(10, request) == expected_result @skip_unless_lms diff --git a/openedx/core/djangoapps/catalog/utils.py b/openedx/core/djangoapps/catalog/utils.py index 68cb9505a305..5b2264c1d6e1 100644 --- a/openedx/core/djangoapps/catalog/utils.py +++ b/openedx/core/djangoapps/catalog/utils.py @@ -29,6 +29,7 @@ SITE_PROGRAM_UUIDS_CACHE_KEY_TPL, ) from openedx.core.djangoapps.catalog.models import CatalogIntegration +from openedx.core.djangoapps.geoinfo.api import country_code_for_request from openedx.core.djangoapps.oauth_dispatch.jwt import create_jwt_for_user from openedx.core.lib.edx_api_utils import get_api_data @@ -366,8 +367,7 @@ def get_localized_price_text(price, request): """ user_currency = {"symbol": "$", "rate": 1, "code": "USD"} - # session.country_code is added via CountryMiddleware in the LMS - user_location = getattr(request, "session", {}).get("country_code") + user_location = country_code_for_request(request) # Override default user_currency if location is available if user_location and get_currency_data: diff --git a/openedx/core/djangoapps/geoinfo/api.py b/openedx/core/djangoapps/geoinfo/api.py index 76ca9f3f2fad..4c61c8fa2e77 100644 --- a/openedx/core/djangoapps/geoinfo/api.py +++ b/openedx/core/djangoapps/geoinfo/api.py @@ -4,6 +4,9 @@ import geoip2.database from django.conf import settings +from python_ipware import IpWare + +_REQUEST_COUNTRY_CODE_ATTR = '_geoinfo_country_code' def country_code_from_ip(ip_addr: str) -> str: @@ -27,3 +30,23 @@ def country_code_from_ip(ip_addr: str) -> str: country_code = "" reader.close() return country_code + + +def country_code_for_request(request) -> str: + """ + Return the country code for the client IP address of a request. + + The lookup runs at most once per request and is not stored in the session, + so requests that don't otherwise use the session don't create or modify one. + + Returns: + A 2-letter country code, or an empty string if the client IP is missing, + not globally routable, or not found. + """ + if not hasattr(request, _REQUEST_COUNTRY_CODE_ATTR): + ip_address, _ = IpWare().get_client_ip(meta=request.META) + country_code = "" + if ip_address and ip_address.is_global: + country_code = country_code_from_ip(format(ip_address)) + setattr(request, _REQUEST_COUNTRY_CODE_ATTR, country_code) + return getattr(request, _REQUEST_COUNTRY_CODE_ATTR) diff --git a/openedx/core/djangoapps/geoinfo/middleware.py b/openedx/core/djangoapps/geoinfo/middleware.py deleted file mode 100644 index ff9ed271bbd1..000000000000 --- a/openedx/core/djangoapps/geoinfo/middleware.py +++ /dev/null @@ -1,47 +0,0 @@ -""" -Middleware to identify the country of origin of page requests. - -Middleware adds `country_code` in session. - -Usage: - -# To enable the Geoinfo feature on a per-view basis, use: -decorator `django.utils.decorators.decorator_from_middleware(middleware_class)` - -""" -import logging - -from django.utils.deprecation import MiddlewareMixin -from python_ipware import IpWare - -from .api import country_code_from_ip - -log = logging.getLogger(__name__) - - -class CountryMiddleware(MiddlewareMixin): - """ - Identify the country by IP address. - """ - def process_request(self, request): - """ - Identify the country by IP address. - - Store country code in session. - """ - ipw = IpWare() - new_ip_address_obj, _ = ipw.get_client_ip(meta=request.META) - - if new_ip_address_obj: - new_ip_address = format(new_ip_address_obj) - - old_ip_address = request.session.get('ip_address', None) - - if not new_ip_address and old_ip_address: - del request.session['ip_address'] - del request.session['country_code'] - elif new_ip_address != old_ip_address and new_ip_address_obj.is_global: - country_code = country_code_from_ip(new_ip_address) - request.session['country_code'] = country_code - request.session['ip_address'] = new_ip_address - log.debug('Country code for IP: %s is set to %s', new_ip_address, country_code) diff --git a/openedx/core/djangoapps/geoinfo/tests/test_api.py b/openedx/core/djangoapps/geoinfo/tests/test_api.py new file mode 100644 index 000000000000..910d545b8dc2 --- /dev/null +++ b/openedx/core/djangoapps/geoinfo/tests/test_api.py @@ -0,0 +1,77 @@ +""" +Tests for the geoinfo API. +""" + + +from unittest.mock import MagicMock, PropertyMock, patch + +import ddt +import geoip2 +import maxminddb +from django.contrib.sessions.middleware import SessionMiddleware +from django.test import TestCase +from django.test.client import RequestFactory + +from openedx.core.djangoapps.geoinfo.api import country_code_for_request + + +@ddt.ddt +class CountryCodeForRequestTests(TestCase): + """ + Tests of country_code_for_request. + """ + def setUp(self): + super().setUp() + self.request_factory = RequestFactory() + patcher = patch.object(maxminddb, 'open_database') + patcher.start() + self.country_mock = MagicMock(side_effect=self.mock_country) + country_patcher = patch.object(geoip2.database.Reader, 'country', self.country_mock) + country_patcher.start() + self.addCleanup(patcher.stop) + self.addCleanup(country_patcher.stop) + + def mock_country(self, ip_address): + """ + Return a mock geoip2 country response for the given IP address. + """ + ip_dict = { + '117.79.83.1': 'CN', + '4.0.0.0': 'SD', + '2001:da8:20f:1502:edcf:550b:4a9c:207d': 'CN', + } + + magic_mock = MagicMock() + magic_mock.country = MagicMock() + type(magic_mock.country).iso_code = PropertyMock(return_value=ip_dict.get(ip_address)) + + return magic_mock + + @ddt.data( + ('117.79.83.1', 'CN'), + ('4.0.0.0', 'SD'), + ('2001:da8:20f:1502:edcf:550b:4a9c:207d', 'CN'), + ('8.8.8.8', ''), + ) + @ddt.unpack + def test_country_code(self, ip_address, expected_country_code): + request = self.request_factory.get('/somewhere', HTTP_X_FORWARDED_FOR=ip_address) + assert country_code_for_request(request) == expected_country_code + + def test_non_global_ip_address_is_not_looked_up(self): + request = self.request_factory.get('/somewhere', HTTP_X_FORWARDED_FOR='10.0.0.1') + assert country_code_for_request(request) == '' + self.country_mock.assert_not_called() + + def test_lookup_runs_once_per_request(self): + request = self.request_factory.get('/somewhere', HTTP_X_FORWARDED_FOR='117.79.83.1') + assert country_code_for_request(request) == 'CN' + assert country_code_for_request(request) == 'CN' + self.country_mock.assert_called_once_with('117.79.83.1') + + def test_session_is_not_modified(self): + request = self.request_factory.get('/somewhere', HTTP_X_FORWARDED_FOR='117.79.83.1') + SessionMiddleware(get_response=lambda request: None).process_request(request) + assert country_code_for_request(request) == 'CN' + assert not request.session.modified + assert request.session.is_empty() diff --git a/openedx/core/djangoapps/geoinfo/tests/test_middleware.py b/openedx/core/djangoapps/geoinfo/tests/test_middleware.py deleted file mode 100644 index d309ead4732a..000000000000 --- a/openedx/core/djangoapps/geoinfo/tests/test_middleware.py +++ /dev/null @@ -1,124 +0,0 @@ -""" -Tests for CountryMiddleware. -""" - - -from unittest.mock import MagicMock, PropertyMock, patch - -import geoip2 -import maxminddb -from django.contrib.sessions.middleware import SessionMiddleware -from django.test import TestCase -from django.test.client import RequestFactory - -from common.djangoapps.student.tests.factories import AnonymousUserFactory, UserFactory -from openedx.core.djangoapps.geoinfo.middleware import CountryMiddleware - - -class CountryMiddlewareTests(TestCase): - """ - Tests of CountryMiddleware. - """ - def setUp(self): - super().setUp() - self.country_middleware = CountryMiddleware(get_response=lambda request: None) - self.session_middleware = SessionMiddleware(get_response=lambda request: None) - self.authenticated_user = UserFactory.create() - self.anonymous_user = AnonymousUserFactory.create() - self.request_factory = RequestFactory() - patcher = patch.object(maxminddb, 'open_database') - patcher.start() - country_patcher = patch.object(geoip2.database.Reader, 'country', self.mock_country) - country_patcher.start() - self.addCleanup(patcher.stop) - self.addCleanup(country_patcher.stop) - - def mock_country(self, ip_address): - """ - :param ip_address: - :return: - """ - ip_dict = { - '117.79.83.1': 'CN', - '117.79.83.100': 'CN', - '4.0.0.0': 'SD', - '2001:da8:20f:1502:edcf:550b:4a9c:207d': 'CN', - } - - magic_mock = MagicMock() - magic_mock.country = MagicMock() - type(magic_mock.country).iso_code = PropertyMock(return_value=ip_dict.get(ip_address)) - - return magic_mock - - def test_country_code_added(self): - request = self.request_factory.get( - '/somewhere', - HTTP_X_FORWARDED_FOR='117.79.83.1', - ) - request.user = self.authenticated_user - self.session_middleware.process_request(request) - # No country code exists before request. - assert 'country_code' not in request.session - assert 'ip_address' not in request.session - self.country_middleware.process_request(request) - # Country code added to session. - assert 'CN' == request.session.get('country_code') - assert '117.79.83.1' == request.session.get('ip_address') - - def test_ip_address_changed(self): - request = self.request_factory.get( - '/somewhere', - HTTP_X_FORWARDED_FOR='4.0.0.0', - ) - request.user = self.anonymous_user - self.session_middleware.process_request(request) - request.session['country_code'] = 'CN' - request.session['ip_address'] = '117.79.83.1' - self.country_middleware.process_request(request) - # Country code is changed. - assert 'SD' == request.session.get('country_code') - assert '4.0.0.0' == request.session.get('ip_address') - - def test_ip_address_is_not_changed(self): - request = self.request_factory.get( - '/somewhere', - HTTP_X_FORWARDED_FOR='117.79.83.1', - ) - request.user = self.anonymous_user - self.session_middleware.process_request(request) - request.session['country_code'] = 'CN' - request.session['ip_address'] = '117.79.83.1' - self.country_middleware.process_request(request) - # Country code is not changed. - assert 'CN' == request.session.get('country_code') - assert '117.79.83.1' == request.session.get('ip_address') - - def test_same_country_different_ip(self): - request = self.request_factory.get( - '/somewhere', - HTTP_X_FORWARDED_FOR='117.79.83.100', - ) - request.user = self.anonymous_user - self.session_middleware.process_request(request) - request.session['country_code'] = 'CN' - request.session['ip_address'] = '117.79.83.1' - self.country_middleware.process_request(request) - # Country code is not changed. - assert 'CN' == request.session.get('country_code') - assert '117.79.83.100' == request.session.get('ip_address') - - def test_ip_address_is_ipv6(self): - request = self.request_factory.get( - '/somewhere', - HTTP_X_FORWARDED_FOR='2001:da8:20f:1502:edcf:550b:4a9c:207d' - ) - request.user = self.authenticated_user - self.session_middleware.process_request(request) - # No country code exists before request. - assert 'country_code' not in request.session - assert 'ip_address' not in request.session - self.country_middleware.process_request(request) - # Country code added to session. - assert 'CN' == request.session.get('country_code') - assert '2001:da8:20f:1502:edcf:550b:4a9c:207d' == request.session.get('ip_address') diff --git a/xmodule/video_block/video_block.py b/xmodule/video_block/video_block.py index 955ecad66363..03780251cae8 100644 --- a/xmodule/video_block/video_block.py +++ b/xmodule/video_block/video_block.py @@ -341,7 +341,6 @@ def get_html(self, view=STUDENT_VIEW, context=None): # pylint: disable=argument # If the user comes from China use China CDN for html5 videos. # 'CN' is China ISO 3166-1 country code. # Video caching is disabled for Studio. User_location is always None in Studio. - # CountryMiddleware disabled for Studio. if getattr(self, 'video_speed_optimizations', True) and cdn_url: if self.edx_video_id and edxval_api and video_status != 'external':