From 94441861e011412822574285bc4cb8d8f81f28af Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Fri, 1 Dec 2023 09:44:59 -0500 Subject: [PATCH] feat!: Remove legacy-ip code and Waffle switch (#33735) This switch has been kept disabled in edx.org for well over a year with no trouble, and the migration to `CLOSEST_CLIENT_IP_FROM_HEADERS` was introduced in Nutmeg. DEPR issue: https://github.com/openedx/edx-platform/issues/33733 --- openedx/core/djangoapps/embargo/api.py | 6 +- openedx/core/djangoapps/embargo/middleware.py | 9 +- .../core/djangoapps/embargo/tests/test_api.py | 15 ++-- .../embargo/tests/test_middleware.py | 82 ------------------- openedx/core/djangoapps/util/legacy_ip.py | 48 ----------- openedx/core/djangoapps/util/ratelimit.py | 7 +- .../djangoapps/util/tests/test_legacy_ip.py | 50 ----------- .../djangoapps/util/tests/test_ratelimit.py | 14 ---- .../core/lib/x_forwarded_for/middleware.py | 20 +---- 9 files changed, 13 insertions(+), 238 deletions(-) delete mode 100644 openedx/core/djangoapps/util/legacy_ip.py delete mode 100644 openedx/core/djangoapps/util/tests/test_legacy_ip.py diff --git a/openedx/core/djangoapps/embargo/api.py b/openedx/core/djangoapps/embargo/api.py index d7c58b6d82fc..0934d13aab63 100644 --- a/openedx/core/djangoapps/embargo/api.py +++ b/openedx/core/djangoapps/embargo/api.py @@ -20,7 +20,6 @@ from common.djangoapps.student.auth import has_course_author_access from openedx.core import types from openedx.core.djangoapps.geoinfo.api import country_code_from_ip -from openedx.core.djangoapps.util import legacy_ip from .models import CountryAccessRule, RestrictedCourse @@ -49,10 +48,7 @@ def redirect_if_blocked( If blocked, a URL path to a page explaining why the user was blocked. Else None. """ if settings.FEATURES.get('EMBARGO'): - if legacy_ip.USE_LEGACY_IP.is_enabled(): - client_ips = [legacy_ip.get_legacy_ip(request)] - else: - client_ips = ip.get_all_client_ips(request) + client_ips = ip.get_all_client_ips(request) user = user or request.user is_blocked = not check_course_access(course_key, user=user, ip_addresses=client_ips, url=request.path) if is_blocked: diff --git a/openedx/core/djangoapps/embargo/middleware.py b/openedx/core/djangoapps/embargo/middleware.py index 51df8d2ed2f5..d35dba11b2fd 100644 --- a/openedx/core/djangoapps/embargo/middleware.py +++ b/openedx/core/djangoapps/embargo/middleware.py @@ -39,7 +39,6 @@ from rest_framework.request import Request from rest_framework.response import Response -from openedx.core.djangoapps.util import legacy_ip from openedx.core.lib.request_utils import course_id_from_url from . import api as embargo_api @@ -87,12 +86,8 @@ def process_request(self, request: Request) -> Optional[Response]: if pattern.match(request.path) is not None: return None - if legacy_ip.USE_LEGACY_IP.is_enabled(): - safest_ip_address = legacy_ip.get_legacy_ip(request) - all_ip_addresses = [safest_ip_address] - else: - safest_ip_address = ip.get_safest_client_ip(request) - all_ip_addresses = ip.get_all_client_ips(request) + safest_ip_address = ip.get_safest_client_ip(request) + all_ip_addresses = ip.get_all_client_ips(request) ip_filter = IPFilter.current() diff --git a/openedx/core/djangoapps/embargo/tests/test_api.py b/openedx/core/djangoapps/embargo/tests/test_api.py index 8459337efde1..4ebb3999f925 100644 --- a/openedx/core/djangoapps/embargo/tests/test_api.py +++ b/openedx/core/djangoapps/embargo/tests/test_api.py @@ -16,7 +16,6 @@ from django.db import connection from django.test.client import RequestFactory from django.test.utils import override_settings -from edx_toggles.toggles.testutils import override_waffle_switch from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, mixed_store_config @@ -28,7 +27,6 @@ OrgStaffRole, OrgInstructorRole ) from common.djangoapps.util.testing import UrlResetMixin -from openedx.core.djangoapps.util.legacy_ip import USE_LEGACY_IP from ..models import ( RestrictedCourse, Country, CountryAccessRule, @@ -234,15 +232,13 @@ def test_staff_access_country_block(self, staff_role_cls): @ddt.data( # (Note that any '0.x.x.x' IP _should_ be blocked in this test.) - # ips, legacy, allow access - (['0.0.0.0', '1.1.1.1'], True, False), # legacy looks at first IP and blocks - (['1.1.1.1', '0.0.0.0'], True, True), # legacy fails to look at later IPs and allows - (['1.1.1.1', '2.2.2.2'], False, True), # normal chain of access - (['1.1.1.1', '0.0.0.0', '2.2.2.2'], False, False), # tried to sneak a blocked IP in, but we caught it + # ips, allow access + (['1.1.1.1', '2.2.2.2'], True), # normal chain of access + (['1.1.1.1', '0.0.0.0', '2.2.2.2'], False), # tried to sneak a blocked IP in, but we caught it ) @ddt.unpack @mock.patch('openedx.core.djangoapps.embargo.api.country_code_from_ip') - def test_redirect_if_blocked_ips(self, ips, use_legacy, allow_access, mock_country): + def test_redirect_if_blocked_ips(self, ips, allow_access, mock_country): # Block the US CountryAccessRule.objects.create( rule_type=CountryAccessRule.BLACKLIST_RULE, @@ -256,8 +252,7 @@ def test_redirect_if_blocked_ips(self, ips, use_legacy, allow_access, mock_count request = RequestFactory().get('', HTTP_X_FORWARDED_FOR=','.join(ips)) request.user = self.user - with override_waffle_switch(USE_LEGACY_IP, use_legacy): - assert (embargo_api.redirect_if_blocked(request, self.course.id) is None) == allow_access + assert (embargo_api.redirect_if_blocked(request, self.course.id) is None) == allow_access @ddt.data( # access point, check disabled, allow access diff --git a/openedx/core/djangoapps/embargo/tests/test_middleware.py b/openedx/core/djangoapps/embargo/tests/test_middleware.py index 61a81c44ae81..9fa1f5b87c5f 100644 --- a/openedx/core/djangoapps/embargo/tests/test_middleware.py +++ b/openedx/core/djangoapps/embargo/tests/test_middleware.py @@ -9,13 +9,11 @@ from django.conf import settings from django.core.cache import cache as django_cache from django.urls import reverse -from edx_toggles.toggles.testutils import override_waffle_switch from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory from common.djangoapps.student.tests.factories import UserFactory from common.djangoapps.util.testing import UrlResetMixin -from openedx.core.djangoapps.util.legacy_ip import USE_LEGACY_IP from openedx.core.djangolib.testing.utils import skip_unless_lms from ..models import IPFilter, RestrictedCourse @@ -161,86 +159,6 @@ def test_ip_whitelist_rules(self, request_ips, blacklist, whitelist, is_enabled, ) self.assertRedirects(response, redirect_url) - @patch.dict(settings.FEATURES, {'EMBARGO': True}) - @override_waffle_switch(USE_LEGACY_IP, True) - @ddt.data( - # request ip chain, blacklist, whitelist, allow_access - (['192.178.2.3'], [], [], False), # confirm that test setup & no config blocks users by default - (['173.194.123.35', '192.178.2.3'], [], ['192.178.2.3'], False), # whitelist ignores last (safest) ip - (['173.194.123.35', '192.178.2.3'], [], ['173.194.0.0/16'], True), # whitelist does look at first ip though - ) - @ddt.unpack - def test_ip_legacy_whitelist_rules(self, request_ips, blacklist, whitelist, allow_access): - # Ensure that IP blocking works for anonymous users - self.client.logout() - - # Set up the IP rules - IPFilter.objects.create( - blacklist=", ".join(blacklist), - whitelist=", ".join(whitelist), - enabled=True, - ) - - # Check that access is enforced (restrict course by default, so that allow-list logic is actually tested) - with restrict_course(self.course.id): - response = self.client.get( - self.courseware_url, - HTTP_X_FORWARDED_FOR=','.join(request_ips), - REMOTE_ADDR=request_ips[-1], - ) - - if allow_access: - assert response.status_code == 200 - else: - redirect_url = reverse( - 'embargo:blocked_message', - kwargs={ - 'access_point': 'courseware', - 'message_key': 'default', - } - ) - self.assertRedirects(response, redirect_url) - - @patch.dict(settings.FEATURES, {'EMBARGO': True}) - @override_waffle_switch(USE_LEGACY_IP, True) - @ddt.data( - # request ip chain, blacklist, whitelist, allow_access - (['192.178.2.3'], [], [], True), # confirm that test setup & no config allows users by default - (['173.194.123.35', '192.178.2.3'], ['192.178.2.3'], [], True), # blacklist ignores last (safest) ip - (['173.194.123.35', '192.178.2.3'], ['173.194.123.35'], [], False), # blacklist looks at first though - (['192.178.2.3'], ['192.178.2.3'], ['192.178.2.3'], False), # blacklist overrides whitelist - ) - @ddt.unpack - def test_ip_legacy_blacklist_rules(self, request_ips, blacklist, whitelist, allow_access): - # Ensure that IP blocking works for anonymous users - self.client.logout() - - # Set up the IP rules - IPFilter.objects.create( - blacklist=", ".join(blacklist), - whitelist=", ".join(whitelist), - enabled=True, - ) - - # Check that access is enforced - response = self.client.get( - self.courseware_url, - HTTP_X_FORWARDED_FOR=','.join(request_ips), - REMOTE_ADDR=request_ips[-1], - ) - - if allow_access: - assert response.status_code == 200 - else: - redirect_url = reverse( - 'embargo:blocked_message', - kwargs={ - 'access_point': 'courseware', - 'message_key': 'embargo', - } - ) - self.assertRedirects(response, redirect_url) - @patch.dict(settings.FEATURES, {'EMBARGO': True}) @ddt.data( ('courseware', 'default'), diff --git a/openedx/core/djangoapps/util/legacy_ip.py b/openedx/core/djangoapps/util/legacy_ip.py deleted file mode 100644 index 5f4f0b16dde7..000000000000 --- a/openedx/core/djangoapps/util/legacy_ip.py +++ /dev/null @@ -1,48 +0,0 @@ -""" -Utilities for migrating to safer IP address determination. - -This module used to contain utilities for reading the IP addresses of -a request, but those have since moved ``edx_django_utils.ip``. - -What remains are the "legacy IP" utils, which should be used only -temporarily when switching a piece of code from using the leftmost IP -(legacy IP) to using the safest IP or full public IP chain (using -edx-django-utils). -""" - -from edx_toggles.toggles import WaffleSwitch - -# .. toggle_name: ip.legacy -# .. toggle_implementation: WaffleSwitch -# .. toggle_default: False -# .. toggle_description: Emergency switch to revert to use an older, less secure method for -# IP determination (instead of the newer, safer code in ``edx_django_utils.ip``). -# When enabled, instructs switch's callers to revert to using the *leftmost* -# IP from the X-Forwarded-For header. When disabled (the default), callers should use the new -# code path for IP determination, which has callers retrieve the entire external chain or pick -# the leftmost or rightmost IP from it. The construction of the external chain is configurable -# via ``CLOSEST_CLIENT_IP_FROM_HEADERS``. -# This toggle, as well as any other legacy IP references, should be deleted (in the off -# position) when the new IP code is well-tested and all IP-reliant code has been switched over -# to using ``edx_django_utils.ip``. -# .. toggle_warning: This switch does not globally control handling of IP addresses; it only -# affects code that is explicitly querying the switch and using ``get_legacy_ip``. -# .. toggle_use_cases: temporary -# .. toggle_creation_date: 2022-03-24 -# .. toggle_target_removal_date: 2023-01-01 -# .. toggle_tickets: https://2u-internal.atlassian.net/browse/ARCHBOM-2056 (internal only) -USE_LEGACY_IP = WaffleSwitch('ip.legacy', module_name=__name__) - - -def get_legacy_ip(request): - """ - Return a client IP selected using an old, insecure method. - - Always picks the leftmost IP in the X-Forwarded-For header, if present, - otherwise returns the original REMOTE_ADDR. - """ - if xff := request.META.get('HTTP_X_FORWARDED_FOR'): - return xff.split(',')[0].strip() - else: - # Might run before or after XForwardedForMiddleware. - return request.META.get('ORIGINAL_REMOTE_ADDR', request.META['REMOTE_ADDR']) diff --git a/openedx/core/djangoapps/util/ratelimit.py b/openedx/core/djangoapps/util/ratelimit.py index a4d1a2a8d6a6..aea9a54d252e 100644 --- a/openedx/core/djangoapps/util/ratelimit.py +++ b/openedx/core/djangoapps/util/ratelimit.py @@ -5,8 +5,6 @@ from edx_django_utils import ip -from openedx.core.djangoapps.util import legacy_ip - def real_ip(group, request): # pylint: disable=unused-argument """ @@ -17,10 +15,7 @@ def real_ip(group, request): # pylint: disable=unused-argument (Intended to be called by ``django-ratelimit``, hence the unused argument.) """ - if legacy_ip.USE_LEGACY_IP.is_enabled(): - return legacy_ip.get_legacy_ip(request) - else: - return ip.get_safest_client_ip(request) + return ip.get_safest_client_ip(request) def request_post_email(group, request) -> str: # pylint: disable=unused-argument diff --git a/openedx/core/djangoapps/util/tests/test_legacy_ip.py b/openedx/core/djangoapps/util/tests/test_legacy_ip.py deleted file mode 100644 index 9619da2868bc..000000000000 --- a/openedx/core/djangoapps/util/tests/test_legacy_ip.py +++ /dev/null @@ -1,50 +0,0 @@ -""" -Tests for IP determination. - -Fake data used in these tests, for consistency: - -- 1.2.3.4 -- a "real" client IP, e.g. the IP of a laptop or phone. -- 127.0.0.2 -- a local reverse proxy (e.g. nginx or caddy) -- 10.0.3.0 -- our load balancer -- 5.5.5.5 -- our CDN -- 6.6.6.6 -- a malicious CDN configuration -- 7.8.9.0 -- something beyond the real client in the IP chain, probably a spoofed header - -...as well as IPv6 versions of these, e.g. 1:2:3:4:: and ::1. - -XXXXXXXXX is used as a standin for anything unparseable (some kind of garbage). -""" - -import ddt -from django.test import TestCase -from django.test.client import RequestFactory - -from openedx.core.djangoapps.util import legacy_ip -from openedx.core.lib.x_forwarded_for.middleware import XForwardedForMiddleware - - -@ddt.ddt -class TestClientIP(TestCase): - """Tests for get_client_ip and helpers.""" - - def setUp(self): - super().setUp() - self.request = RequestFactory().get('/somewhere') - - @ddt.unpack - @ddt.data( - ( - {'HTTP_X_FORWARDED_FOR': '7.8.9.0, 1.2.3.4, 10.0.3.0', 'REMOTE_ADDR': '0:0:0:0::1'}, - '7.8.9.0', - ), - - # XFF is not required - ({'REMOTE_ADDR': '127.0.0.2'}, '127.0.0.2'), - ) - def test_get_legacy_ip(self, request_meta, expected): - self.request.META = request_meta - assert legacy_ip.get_legacy_ip(self.request) == expected - - # Check that it still works after the XFF middleware has done its dirty work - XForwardedForMiddleware(get_response=lambda request: None).process_request(self.request) - assert legacy_ip.get_legacy_ip(self.request) == expected diff --git a/openedx/core/djangoapps/util/tests/test_ratelimit.py b/openedx/core/djangoapps/util/tests/test_ratelimit.py index 1fe6d18d4233..cdf0a588cddc 100644 --- a/openedx/core/djangoapps/util/tests/test_ratelimit.py +++ b/openedx/core/djangoapps/util/tests/test_ratelimit.py @@ -5,10 +5,8 @@ import ddt from django.test import TestCase from django.test.client import RequestFactory -from edx_toggles.toggles.testutils import override_waffle_switch import openedx.core.djangoapps.util.ratelimit as ratelimit -from openedx.core.djangoapps.util.legacy_ip import USE_LEGACY_IP from openedx.core.lib.x_forwarded_for.middleware import XForwardedForMiddleware @@ -36,18 +34,6 @@ def test_real_ip_after_xff_middleware(self): XForwardedForMiddleware(get_response=lambda request: None).process_request(self.request) assert ratelimit.real_ip(None, self.request) == '1.2.3.4' - @override_waffle_switch(USE_LEGACY_IP, True) - def test_legacy_switch(self): - assert ratelimit.real_ip(None, self.request) == '7.8.9.0' - - @override_waffle_switch(USE_LEGACY_IP, True) - def test_legacy_switch_after_xff_middleware(self): - """ - Again, but with XFF Middleware running first. - """ - XForwardedForMiddleware(get_response=lambda request: None).process_request(self.request) - assert ratelimit.real_ip(None, self.request) == '7.8.9.0' - def test_request_post_email(self): """ Tests post email param. diff --git a/openedx/core/lib/x_forwarded_for/middleware.py b/openedx/core/lib/x_forwarded_for/middleware.py index 9682f14bbe2d..cd208809820e 100644 --- a/openedx/core/lib/x_forwarded_for/middleware.py +++ b/openedx/core/lib/x_forwarded_for/middleware.py @@ -8,8 +8,6 @@ from edx_django_utils import ip from edx_django_utils.monitoring import set_custom_attribute -from openedx.core.djangoapps.util import legacy_ip - def _ip_type(ip_str): """ @@ -45,9 +43,6 @@ def process_request(self, request): # This function will cache its results in the request. ip.init_client_ips(request) - # Only used to support ip.legacy switch. - request.META['ORIGINAL_REMOTE_ADDR'] = request.META['REMOTE_ADDR'] - safest_client_ip = ip.get_safest_client_ip(request) try: @@ -63,8 +58,6 @@ def process_request(self, request): set_custom_attribute('ip_chain.count', len(ip_chain)) set_custom_attribute('ip_chain.types', '-'.join(_ip_type(s) for s in ip_chain)) - set_custom_attribute('ip_chain.use_legacy', legacy_ip.USE_LEGACY_IP.is_enabled()) - external_chain = ip.get_all_client_ips(request) set_custom_attribute('ip_chain.external.count', len(external_chain)) set_custom_attribute('ip_chain.external.types', '-'.join(_ip_type(s) for s in external_chain)) @@ -102,13 +95,8 @@ def process_request(self, request): # makes it possible to handle multi-valued headers correctly. # After that, this override can probably be safely removed. # - # It is very important that init_client_ips is called before this + # It is important that init_client_ips is called before this # point, allowing it to cache its results in request.META, since - # after this point it will be more difficult for it to operate - # without knowing about ORIGINAL_REMOTE_ADDR. (The less code that - # is aware of that, the better, and the ip code should be lifted - # out into a library anyhow.) - if legacy_ip.USE_LEGACY_IP.is_enabled(): - request.META['REMOTE_ADDR'] = legacy_ip.get_legacy_ip(request) - else: - request.META['REMOTE_ADDR'] = safest_client_ip + # after this point it will be unable to reconstruct the original + # IP chain. + request.META['REMOTE_ADDR'] = safest_client_ip