Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat!: Remove legacy-ip code and Waffle switch (#33735)

Browse Source 
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
This commit is contained in:
Tim McCormack
2023-12-01 09:44:59 -05:00
committed by GitHub
parent 29a0edd7aa
commit 94441861e0
9 changed files with 13 additions and 238 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
openedx/core/djangoapps/embargo/api.py
View File

@@ -20,7 +20,6 @@ from rest_framework.response import Response
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:

9
openedx/core/djangoapps/embargo/middleware.py
View File

@@ -39,7 +39,6 @@ from edx_django_utils import ip
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 @@ class EmbargoMiddleware(MiddlewareMixin):
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()

15
openedx/core/djangoapps/embargo/tests/test_api.py
View File

@@ -16,7 +16,6 @@ from django.core.cache import cache
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 @@ from common.djangoapps.student.roles import (
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 @@ class EmbargoCheckAccessApiTests(ModuleStoreTestCase):
@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 @@ class EmbargoCheckAccessApiTests(ModuleStoreTestCase):
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

82
openedx/core/djangoapps/embargo/tests/test_middleware.py
View File

@@ -9,13 +9,11 @@ from config_models.models import cache as config_cache
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 @@ class EmbargoMiddlewareAccessTests(UrlResetMixin, ModuleStoreTestCase):
)
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'),

48
openedx/core/djangoapps/util/legacy_ip.py
View File

@@ -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'])

7
openedx/core/djangoapps/util/ratelimit.py
View File

@@ -5,8 +5,6 @@ from uuid import uuid4
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

50
openedx/core/djangoapps/util/tests/test_legacy_ip.py
View File

@@ -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

14
openedx/core/djangoapps/util/tests/test_ratelimit.py
View File

@@ -5,10 +5,8 @@ Tests for rate-limiting.
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 @@ class TestRateLimiting(TestCase):
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.

20
openedx/core/lib/x_forwarded_for/middleware.py
View File

@@ -8,8 +8,6 @@ from django.utils.deprecation import MiddlewareMixin
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 @@ class XForwardedForMiddleware(MiddlewareMixin):
# 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 @@ class XForwardedForMiddleware(MiddlewareMixin):
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 @@ class XForwardedForMiddleware(MiddlewareMixin):
# 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