From 7686014144930bb9fda4ae8beb66f0392855e55a Mon Sep 17 00:00:00 2001 From: attiyaIshaque Date: Tue, 4 Jan 2022 13:32:11 +0500 Subject: [PATCH 01/31] fix: Add security fix in LMS logout redirect_url. --- .../djangoapps/user_authn/views/logout.py | 3 ++- .../user_authn/views/tests/test_logout.py | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/openedx/core/djangoapps/user_authn/views/logout.py b/openedx/core/djangoapps/user_authn/views/logout.py index ef8e3bf59af..4d88433a4f2 100644 --- a/openedx/core/djangoapps/user_authn/views/logout.py +++ b/openedx/core/djangoapps/user_authn/views/logout.py @@ -5,6 +5,7 @@ import urllib.parse as parse # pylint: disable=import-error from urllib.parse import parse_qs, urlsplit, urlunsplit # pylint: disable=import-error +import bleach from django.conf import settings from django.contrib.auth import logout from django.utils.http import urlencode @@ -58,7 +59,7 @@ def target(self): # >> /courses/course-v1:ARTS+D1+2018_T/course/ # to handle this scenario we need to encode our URL using quote_plus and then unquote it again. if target_url: - target_url = parse.unquote(parse.quote_plus(target_url)) + target_url = bleach.clean(parse.unquote(parse.quote_plus(target_url))) use_target_url = target_url and is_safe_login_or_logout_redirect( redirect_to=target_url, diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_logout.py b/openedx/core/djangoapps/user_authn/views/tests/test_logout.py index 6e906ca53f0..2a458cb4fce 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_logout.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_logout.py @@ -7,6 +7,7 @@ import urllib from unittest import mock import ddt +import bleach from django.conf import settings from django.test import TestCase from django.test.utils import override_settings @@ -193,3 +194,21 @@ def test_learner_portal_logout_having_idp_logout_url(self): 'show_tpa_logout_link': True, } self.assertDictContainsSubset(expected, response.context_data) + + @ddt.data( + ('%22%3E%3Cscript%3Ealert(%27xss%27)%3C/script%3E', 'edx.org'), + ) + @ddt.unpack + def test_logout_redirect_failure_with_xss_vulnerability(self, redirect_url, host): + """ + Verify that it will block the XSS attack on edX’s LMS logout page + """ + url = '{logout_path}?redirect_url={redirect_url}'.format( + logout_path=reverse('logout'), + redirect_url=redirect_url + ) + response = self.client.get(url, HTTP_HOST=host) + expected = { + 'target': bleach.clean(urllib.parse.unquote(redirect_url)), + } + self.assertDictContainsSubset(expected, response.context_data) From d17451c4cbc0d0c4a425a975cab44997c315fca9 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Tue, 29 Mar 2022 18:44:06 +0000 Subject: [PATCH 02/31] fix: updated the unit tests workflow to use labeled runners Backported from master commit 96b2162/PR #29667 --- .github/workflows/unit-tests.yml | 2 +- .github/workflows/verify-gha-unit-tests-count.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 8239e2806ef..eafedcc8285 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -9,7 +9,7 @@ on: jobs: run-tests: - runs-on: [ self-hosted ] + runs-on: [ edx-platform-runner ] strategy: matrix: python-version: ['3.8'] diff --git a/.github/workflows/verify-gha-unit-tests-count.yml b/.github/workflows/verify-gha-unit-tests-count.yml index 38df28180fb..4f9aafbdde2 100644 --- a/.github/workflows/verify-gha-unit-tests-count.yml +++ b/.github/workflows/verify-gha-unit-tests-count.yml @@ -8,7 +8,7 @@ on: jobs: collect-and-verify: - runs-on: [ self-hosted ] + runs-on: [ edx-platform-runner ] steps: - name: sync directory owner run: sudo chown runner:runner -R .* From d1afd4712e3f710da1351dced84c9f649f77204d Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Tue, 29 Mar 2022 18:48:52 +0000 Subject: [PATCH 03/31] build: update mongo server up command in unit tests CI Backport of commit b51f65d from master/ PR #30051 --- .github/workflows/unit-tests.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index eafedcc8285..9c27711d5a7 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -35,9 +35,11 @@ jobs: - name: sync directory owner run: sudo chown runner:runner -R .* - uses: actions/checkout@v2 - - name: start mongodb service + - name: start mongod server for tests run: | - sudo /etc/init.d/mongodb start + sudo mkdir -p /data/db + sudo chmod -R a+rw /data/db + mongod & - name: set top-level module name run: | From 7e37238db1643f57ddd11e7cc1de06062a666431 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Tue, 29 Mar 2022 20:33:12 +0000 Subject: [PATCH 04/31] feat: Allow REST_FRAMEWORK to be configured by (shallow) merge (#30142) ARCHBOM-2073 Backported from master commit 7c7d7d8b/PR #30112. --- cms/envs/production.py | 4 ++++ lms/envs/production.py | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/cms/envs/production.py b/cms/envs/production.py index 0f9740cc4f9..aefd5793bdf 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -74,6 +74,7 @@ def get_env_setting(setting): 'CELERY_QUEUES', 'MKTG_URL_LINK_MAP', 'MKTG_URL_OVERRIDES', + 'REST_FRAMEWORK', ] for key in KEYS_WITH_MERGED_VALUES: if key in __config_copy__: @@ -602,3 +603,6 @@ def get_env_setting(setting): SHOW_ACCOUNT_ACTIVATION_CTA = ENV_TOKENS.get('SHOW_ACCOUNT_ACTIVATION_CTA', SHOW_ACCOUNT_ACTIVATION_CTA) LANGUAGE_COOKIE_NAME = ENV_TOKENS.get('LANGUAGE_COOKIE', None) or ENV_TOKENS.get('LANGUAGE_COOKIE_NAME') + +############## DRF overrides ############## +REST_FRAMEWORK.update(ENV_TOKENS.get('REST_FRAMEWORK', {})) diff --git a/lms/envs/production.py b/lms/envs/production.py index 75a225ca30c..d517908c060 100644 --- a/lms/envs/production.py +++ b/lms/envs/production.py @@ -82,6 +82,7 @@ def get_env_setting(setting): 'CELERY_QUEUES', 'MKTG_URL_LINK_MAP', 'MKTG_URL_OVERRIDES', + 'REST_FRAMEWORK', ] for key in KEYS_WITH_MERGED_VALUES: if key in __config_copy__: @@ -1053,3 +1054,6 @@ def get_env_setting(setting): CHROME_DISABLE_SUBFRAME_DIALOG_SUPPRESSION_TOKEN = ENV_TOKENS.get( 'CHROME_DISABLE_SUBFRAME_DIALOG_SUPPRESSION_TOKEN', CHROME_DISABLE_SUBFRAME_DIALOG_SUPPRESSION_TOKEN ) + +############## DRF overrides ############## +REST_FRAMEWORK.update(ENV_TOKENS.get('REST_FRAMEWORK', {})) From e2b863dbd76868787c366739a0bda93fd11b724f Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Fri, 18 Mar 2022 15:31:27 +0000 Subject: [PATCH 05/31] feat: Add monitoring for full IP chain's length (#30090, #30106) Cherry picked from commits: - c3bc68abc1641e45c45d5160c12c0107fb055ba0 - 813b403575a1c50fe2af657c69f4c7c885e55439 --- .../core/lib/x_forwarded_for/middleware.py | 8 ++ .../lib/x_forwarded_for/tests/__init__.py | 0 .../x_forwarded_for/tests/test_middleware.py | 77 +++++++++++++++++++ 3 files changed, 85 insertions(+) create mode 100644 openedx/core/lib/x_forwarded_for/tests/__init__.py create mode 100644 openedx/core/lib/x_forwarded_for/tests/test_middleware.py diff --git a/openedx/core/lib/x_forwarded_for/middleware.py b/openedx/core/lib/x_forwarded_for/middleware.py index 98a8bf7bf91..162a6e3d21b 100644 --- a/openedx/core/lib/x_forwarded_for/middleware.py +++ b/openedx/core/lib/x_forwarded_for/middleware.py @@ -3,7 +3,9 @@ Updated the libray to use HTTP_HOST and X-Forwarded-Port as SERVER_NAME and SERVER_PORT. """ + from django.utils.deprecation import MiddlewareMixin +from edx_django_utils.monitoring import set_custom_attribute class XForwardedForMiddleware(MiddlewareMixin): @@ -19,6 +21,12 @@ def process_request(self, request): # lint-amnesty, pylint: disable=useless-ret Process the given request, update the value of REMOTE_ADDR, SERVER_NAME and SERVER_PORT based on X-Forwarded-For, HTTP_HOST and X-Forwarded-Port headers """ + # Give some observability into X-Forwarded-For length. Useful + # for monitoring in case of unexpected changes, etc. + xff = request.META.get('HTTP_X_FORWARDED_FOR') + xff_len = xff.count(',') + 1 if xff else 0 + # IP chain is XFF + REMOTE_ADDR + set_custom_attribute('ip_chain.count', xff_len + 1) for field, header in [("HTTP_X_FORWARDED_FOR", "REMOTE_ADDR"), ("HTTP_HOST", "SERVER_NAME"), ("HTTP_X_FORWARDED_PORT", "SERVER_PORT")]: diff --git a/openedx/core/lib/x_forwarded_for/tests/__init__.py b/openedx/core/lib/x_forwarded_for/tests/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/openedx/core/lib/x_forwarded_for/tests/test_middleware.py b/openedx/core/lib/x_forwarded_for/tests/test_middleware.py new file mode 100644 index 00000000000..8fceb2dc5a9 --- /dev/null +++ b/openedx/core/lib/x_forwarded_for/tests/test_middleware.py @@ -0,0 +1,77 @@ +""" +Tests for XForwardedForMiddleware. +""" + +from unittest.mock import call, patch + +import ddt +from django.test import TestCase +from django.test.client import RequestFactory + +from openedx.core.lib.x_forwarded_for.middleware import XForwardedForMiddleware + + +@ddt.ddt +class TestXForwardedForMiddleware(TestCase): + """Tests for middleware's overrides.""" + + @ddt.unpack + @ddt.data( + # With no added headers, just see the test server's defaults. + ( + {}, + { + 'SERVER_NAME': 'testserver', + 'SERVER_PORT': '80', + 'REMOTE_ADDR': '127.0.0.1', + }, + ), + # With headers supplied by the request (Host) and a proxy + # (X-Forwarded-Port), see the name and port overridden. + ( + { + 'HTTP_HOST': 'example.com', + 'HTTP_X_FORWARDED_PORT': '443', + }, + { + 'SERVER_NAME': 'example.com', + 'SERVER_PORT': '443', + }, + ), + + # REMOTE_ADDR can also be overridden + ( + {'HTTP_X_FORWARDED_FOR': '7.8.9.0, 1.2.3.4'}, + { + 'REMOTE_ADDR': '7.8.9.0', + }, + ), + ) + def test_overrides(self, add_meta, expected_meta_include): + """ + Test that parts of request.META can be overridden by HTTP headers. + """ + request = RequestFactory().get('/somewhere') + request.META.update(add_meta) + + XForwardedForMiddleware().process_request(request) + + assert request.META.items() >= expected_meta_include.items() + + @ddt.unpack + @ddt.data( + (None, 1), + ('1.2.3.4', 2), + ('XXXXXXXX, 1.2.3.4, 5.5.5.5', 4), + ) + @patch("openedx.core.lib.x_forwarded_for.middleware.set_custom_attribute") + def test_xff_metrics(self, xff, expected_count, mock_set_custom_attribute): + request = RequestFactory().get('/somewhere') + if xff is not None: + request.META['HTTP_X_FORWARDED_FOR'] = xff + + XForwardedForMiddleware().process_request(request) + + mock_set_custom_attribute.assert_has_calls([ + call('ip_chain.count', expected_count), + ]) From 5ebdee417bccd06e31491e6ea6d4687d75b43054 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Wed, 30 Mar 2022 14:13:18 +0000 Subject: [PATCH 06/31] chore: Disable lint check on a line that the linter is OK with on master The versions of pylint are different on master and Maple; pylint 2.11 stops complaining about this line. Just disable the check for the backported code. --- openedx/core/lib/x_forwarded_for/tests/test_middleware.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openedx/core/lib/x_forwarded_for/tests/test_middleware.py b/openedx/core/lib/x_forwarded_for/tests/test_middleware.py index 8fceb2dc5a9..a3b53c17c7e 100644 --- a/openedx/core/lib/x_forwarded_for/tests/test_middleware.py +++ b/openedx/core/lib/x_forwarded_for/tests/test_middleware.py @@ -56,7 +56,7 @@ def test_overrides(self, add_meta, expected_meta_include): XForwardedForMiddleware().process_request(request) - assert request.META.items() >= expected_meta_include.items() + assert request.META.items() >= expected_meta_include.items() # pylint: disable=dict-items-not-iterating @ddt.unpack @ddt.data( From cd7b45dbd9d14ec49a7c86aa5d1eb302e280de15 Mon Sep 17 00:00:00 2001 From: Rebecca Graber Date: Fri, 8 Apr 2022 11:28:07 -0400 Subject: [PATCH 07/31] fix: verify redirect in inactive_user_view (#30220) --- .../third_party_auth/tests/test_views.py | 58 +++++++++++++++++++ common/djangoapps/third_party_auth/views.py | 12 +++- 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/common/djangoapps/third_party_auth/tests/test_views.py b/common/djangoapps/third_party_auth/tests/test_views.py index 3508a9d941b..10c64579ebb 100644 --- a/common/djangoapps/third_party_auth/tests/test_views.py +++ b/common/djangoapps/third_party_auth/tests/test_views.py @@ -4,17 +4,23 @@ import unittest +from unittest.mock import patch import ddt import pytest from django.conf import settings +from django.test import TestCase, override_settings +from django.test.client import RequestFactory from django.urls import reverse from lxml import etree from onelogin.saml2.errors import OneLogin_Saml2_Error +from common.djangoapps.student.models import Registration +from common.djangoapps.student.tests.factories import UserFactory from common.djangoapps.third_party_auth import pipeline # Define some XML namespaces: from common.djangoapps.third_party_auth.tasks import SAML_XML_NS +from common.djangoapps.third_party_auth.views import inactive_user_view from .testutil import AUTH_FEATURE_ENABLED, AUTH_FEATURES_KEY, SAMLTestCase @@ -196,3 +202,55 @@ def get_idp_redirect_url(provider_slug, next_destination=None): idp_redirect_url=reverse('idp_redirect', kwargs={'provider_slug': provider_slug}), next_destination=next_destination, ) + + +@unittest.skipUnless(AUTH_FEATURE_ENABLED, AUTH_FEATURES_KEY + ' not enabled') +class InactiveUserViewTests(TestCase): + """Test inactive user view """ + @patch('common.djangoapps.third_party_auth.views.redirect') + @override_settings(LOGIN_REDIRECT_WHITELIST=['courses.edx.org']) + def test_inactive_user_view_allows_valid_redirect(self, mock_redirect): + inactive_user = UserFactory(is_active=False) + Registration().register(inactive_user) + request = RequestFactory().get(settings.SOCIAL_AUTH_INACTIVE_USER_URL, {'next': 'https://courses.edx.org'}) + request.user = inactive_user + with patch('common.djangoapps.edxmako.request_context.get_current_request', return_value=request): + with patch('common.djangoapps.third_party_auth.pipeline.running', return_value=False): + inactive_user_view(request) + mock_redirect.assert_called_with('https://courses.edx.org') + + @patch('common.djangoapps.third_party_auth.views.redirect') + def test_inactive_user_view_prevents_invalid_redirect(self, mock_redirect): + inactive_user = UserFactory(is_active=False) + Registration().register(inactive_user) + request = RequestFactory().get(settings.SOCIAL_AUTH_INACTIVE_USER_URL, {'next': 'https://evil.com'}) + request.user = inactive_user + with patch('common.djangoapps.edxmako.request_context.get_current_request', return_value=request): + with patch('common.djangoapps.third_party_auth.pipeline.running', return_value=False): + inactive_user_view(request) + mock_redirect.assert_called_with('dashboard') + + @patch('common.djangoapps.third_party_auth.views.redirect') + def test_inactive_user_view_redirects_back_to_host(self, mock_redirect): + inactive_user = UserFactory(is_active=False) + Registration().register(inactive_user) + request = RequestFactory().get(settings.SOCIAL_AUTH_INACTIVE_USER_URL, {'next': 'https://myedxhost.com'}, + HTTP_HOST='myedxhost.com') + request.user = inactive_user + with patch('common.djangoapps.edxmako.request_context.get_current_request', return_value=request): + with patch('common.djangoapps.third_party_auth.pipeline.running', return_value=False): + inactive_user_view(request) + mock_redirect.assert_called_with('https://myedxhost.com') + + @patch('common.djangoapps.third_party_auth.views.redirect') + @override_settings(LOGIN_REDIRECT_WHITELIST=['courses.edx.org']) + def test_inactive_user_view_does_not_redirect_https_to_http(self, mock_redirect): + inactive_user = UserFactory(is_active=False) + Registration().register(inactive_user) + request = RequestFactory().get(settings.SOCIAL_AUTH_INACTIVE_USER_URL, {'next': 'http://courses.edx.org'}, + secure=True) + request.user = inactive_user + with patch('common.djangoapps.edxmako.request_context.get_current_request', return_value=request): + with patch('common.djangoapps.third_party_auth.pipeline.running', return_value=False): + inactive_user_view(request) + mock_redirect.assert_called_with('dashboard') diff --git a/common/djangoapps/third_party_auth/views.py b/common/djangoapps/third_party_auth/views.py index 3f35a48a326..c6a406c5301 100644 --- a/common/djangoapps/third_party_auth/views.py +++ b/common/djangoapps/third_party_auth/views.py @@ -14,7 +14,7 @@ from social_django.views import complete from common.djangoapps import third_party_auth -from common.djangoapps.student.helpers import get_next_url_for_login_page +from common.djangoapps.student.helpers import get_next_url_for_login_page, is_safe_login_or_logout_redirect from common.djangoapps.student.models import UserProfile from common.djangoapps.student.views import compose_and_send_activation_email from common.djangoapps.third_party_auth import pipeline, provider @@ -54,7 +54,15 @@ def inactive_user_view(request): if not activated: compose_and_send_activation_email(user, profile) - return redirect(request.GET.get('next', 'dashboard')) + request_params = request.GET + redirect_to = request_params.get('next') + + if redirect_to and is_safe_login_or_logout_redirect(redirect_to=redirect_to, request_host=request.get_host(), + dot_client_id=request_params.get('client_id'), + require_https=request.is_secure()): + return redirect(redirect_to) + + return redirect('dashboard') def saml_metadata_view(request): From b8e31d47ed7dd06f53d88167c4063afc433a06fe Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Mon, 11 Apr 2022 09:35:43 -0400 Subject: [PATCH 08/31] chore: upgrade Django to 3.2.13 --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/django.txt | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index be6c99b70ae..16b8ef7004c 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -163,7 +163,7 @@ defusedxml==0.7.1 # social-auth-core deprecated==1.2.13 # via jwcrypto -django==3.2.12 +django==3.2.13 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/base.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 0c9745bd7d2..c0e8f93bf6b 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -243,7 +243,7 @@ distlib==0.3.3 # via # -r requirements/edx/testing.txt # virtualenv -django==3.2.12 +django==3.2.13 # via # -c requirements/edx/../constraints.txt # -r requirements/edx/testing.txt diff --git a/requirements/edx/django.txt b/requirements/edx/django.txt index 15d7c60eba8..09696ec840e 100644 --- a/requirements/edx/django.txt +++ b/requirements/edx/django.txt @@ -1 +1 @@ -django==3.2.12 +django==3.2.13 From ab7aba5235fe4310b72502fdc2470474ed9bca0c Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Fri, 15 Apr 2022 14:03:36 -0400 Subject: [PATCH 09/31] fix: Make SAMLConfiguration viewset readonly (#30260) The only use is a GET request in admin portal so this view need not be post/put friendly right now. It may actually get removed in an upcoming iteration, or stay readonly. Fixes: SEC-1418 Backported from 283141a --- common/djangoapps/third_party_auth/saml_configuration/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/third_party_auth/saml_configuration/views.py b/common/djangoapps/third_party_auth/saml_configuration/views.py index 093717b2962..aa051aac7f9 100644 --- a/common/djangoapps/third_party_auth/saml_configuration/views.py +++ b/common/djangoapps/third_party_auth/saml_configuration/views.py @@ -16,7 +16,7 @@ class SAMLConfigurationMixin: serializer_class = SAMLConfigurationSerializer -class SAMLConfigurationViewSet(SAMLConfigurationMixin, viewsets.ModelViewSet): +class SAMLConfigurationViewSet(SAMLConfigurationMixin, viewsets.ReadOnlyModelViewSet): """ A View to handle SAMLConfiguration GETs From c5eec539a99ba22093718879c774075df6ecb132 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Wed, 20 Apr 2022 16:30:28 +0000 Subject: [PATCH 10/31] test: Ignore Waffle table in query count assertions This is a backport from parts of 29e50710 and 9fa79809; not bringing in all of 8b775961 (rename of blacklist to ignorelist) since that would expand the scope of this PR too much, and the next release is coming up soon anyhow. --- .../certificates/apis/v0/tests/test_views.py | 7 +++--- lms/djangoapps/course_api/tests/test_views.py | 5 ++-- .../user_api/accounts/tests/test_views.py | 25 ++++++++++--------- 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/lms/djangoapps/certificates/apis/v0/tests/test_views.py b/lms/djangoapps/certificates/apis/v0/tests/test_views.py index 731aec06b11..9a56168f9ac 100644 --- a/lms/djangoapps/certificates/apis/v0/tests/test_views.py +++ b/lms/djangoapps/certificates/apis/v0/tests/test_views.py @@ -21,6 +21,7 @@ from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.user_api.tests.factories import UserPreferenceFactory from openedx.core.djangoapps.user_authn.tests.utils import JWT_AUTH_TYPES, AuthAndScopesTestMixin, AuthType +from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory @@ -308,7 +309,7 @@ def test_no_certificate(self): def test_query_counts(self): # Test student with no certificates student_no_cert = UserFactory.create(password=self.user_password) - with self.assertNumQueries(18): + with self.assertNumQueries(17, table_blacklist=WAFFLE_TABLES): resp = self.get_response( AuthType.jwt, requesting_user=self.global_staff, @@ -318,7 +319,7 @@ def test_query_counts(self): assert len(resp.data) == 0 # Test student with 1 certificate - with self.assertNumQueries(12): + with self.assertNumQueries(12, table_blacklist=WAFFLE_TABLES): resp = self.get_response( AuthType.jwt, requesting_user=self.global_staff, @@ -358,7 +359,7 @@ def test_query_counts(self): download_url='www.google.com', grade="0.88", ) - with self.assertNumQueries(12): + with self.assertNumQueries(12, table_blacklist=WAFFLE_TABLES): resp = self.get_response( AuthType.jwt, requesting_user=self.global_staff, diff --git a/lms/djangoapps/course_api/tests/test_views.py b/lms/djangoapps/course_api/tests/test_views.py index e2cb2521c20..5057de88f68 100644 --- a/lms/djangoapps/course_api/tests/test_views.py +++ b/lms/djangoapps/course_api/tests/test_views.py @@ -23,6 +23,7 @@ from common.djangoapps.student.auth import add_users from common.djangoapps.student.roles import CourseInstructorRole, CourseStaffRole from common.djangoapps.student.tests.factories import AdminFactory +from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES from openedx.core.lib.api.view_utils import LazySequence from openedx.features.content_type_gating.models import ContentTypeGatingConfig from openedx.features.course_duration_limits.models import CourseDurationLimitConfig @@ -417,14 +418,14 @@ def test_too_many_courses(self): self.setup_user(self.audit_user) # These query counts were found empirically - query_counts = [54, 46, 46, 46, 46, 46, 46, 46, 46, 46, 16] + query_counts = [50, 46, 46, 46, 46, 46, 46, 46, 46, 46, 16] ordered_course_ids = sorted([str(cid) for cid in (course_ids + [c.id for c in self.courses])]) self.clear_caches() for page in range(1, 12): RequestCache.clear_all_namespaces() - with self.assertNumQueries(query_counts[page - 1]): + with self.assertNumQueries(query_counts[page - 1], table_blacklist=WAFFLE_TABLES): response = self.verify_response(params={'page': page, 'page_size': 30}) assert 'results' in response.data diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py index 2dfcf28f44b..af413f2a4f1 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_views.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_views.py @@ -21,7 +21,8 @@ from openedx.core.djangoapps.user_api.accounts import ACCOUNT_VISIBILITY_PREF_KEY from openedx.core.djangoapps.user_api.models import UserPreference from openedx.core.djangoapps.user_api.preferences.api import set_user_preference -from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms +from openedx.core.djangoapps.waffle_utils.testutils import WAFFLE_TABLES +from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, FilteredQueryCountMixin, skip_unless_lms from common.djangoapps.student.models import PendingEmailChange, UserProfile from common.djangoapps.student.tests.factories import TEST_PASSWORD, UserFactory, RegistrationFactory @@ -166,7 +167,7 @@ def _verify_profile_image_data(self, data, has_profile_image): @ddt.ddt @skip_unless_lms -class TestOwnUsernameAPI(CacheIsolationTestCase, UserAPITestCase): +class TestOwnUsernameAPI(FilteredQueryCountMixin, CacheIsolationTestCase, UserAPITestCase): """ Unit tests for the Accounts API. """ @@ -182,7 +183,7 @@ def _verify_get_own_username(self, queries, expected_status=200): """ Internal helper to perform the actual assertion """ - with self.assertNumQueries(queries): + with self.assertNumQueries(queries, table_blacklist=WAFFLE_TABLES): response = self.send_get(self.client, expected_status=expected_status) if expected_status == 200: data = response.data @@ -194,7 +195,7 @@ def test_get_username(self): Test that a client (logged in) can get her own username. """ self.client.login(username=self.user.username, password=TEST_PASSWORD) - self._verify_get_own_username(17) + self._verify_get_own_username(16) def test_get_username_inactive(self): """ @@ -204,7 +205,7 @@ def test_get_username_inactive(self): self.client.login(username=self.user.username, password=TEST_PASSWORD) self.user.is_active = False self.user.save() - self._verify_get_own_username(17) + self._verify_get_own_username(16) def test_get_username_not_logged_in(self): """ @@ -213,7 +214,7 @@ def test_get_username_not_logged_in(self): """ # verify that the endpoint is inaccessible when not logged in - self._verify_get_own_username(13, expected_status=401) + self._verify_get_own_username(12, expected_status=401) @ddt.ddt @@ -224,7 +225,7 @@ def test_get_username_not_logged_in(self): {'full': 50, 'small': 10}, clear=True ) -class TestAccountsAPI(CacheIsolationTestCase, UserAPITestCase): +class TestAccountsAPI(FilteredQueryCountMixin, CacheIsolationTestCase, UserAPITestCase): """ Unit tests for the Accounts API. """ @@ -467,7 +468,7 @@ def test_get_account_different_user_visible(self): """ self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) self.create_mock_profile(self.user) - with self.assertNumQueries(27): + with self.assertNumQueries(24, table_blacklist=WAFFLE_TABLES): response = self.send_get(self.different_client) self._verify_full_shareable_account_response(response, account_privacy=ALL_USERS_VISIBILITY) @@ -482,7 +483,7 @@ def test_get_account_different_user_private(self): """ self.different_client.login(username=self.different_user.username, password=TEST_PASSWORD) self.create_mock_profile(self.user) - with self.assertNumQueries(27): + with self.assertNumQueries(24, table_blacklist=WAFFLE_TABLES): response = self.send_get(self.different_client) self._verify_private_account_response(response) @@ -604,7 +605,7 @@ def verify_get_own_information(queries): """ Internal helper to perform the actual assertions """ - with self.assertNumQueries(queries): + with self.assertNumQueries(queries, table_blacklist=WAFFLE_TABLES): response = self.send_get(self.client) data = response.data assert 30 == len(data) @@ -630,7 +631,7 @@ def verify_get_own_information(queries): assert data['accomplishments_shared'] is False self.client.login(username=self.user.username, password=TEST_PASSWORD) - verify_get_own_information(25) + verify_get_own_information(22) # Now make sure that the user can get the same information, even if not active self.user.is_active = False @@ -650,7 +651,7 @@ def test_get_account_empty_string(self): legacy_profile.save() self.client.login(username=self.user.username, password=TEST_PASSWORD) - with self.assertNumQueries(25): + with self.assertNumQueries(22, table_blacklist=WAFFLE_TABLES): response = self.send_get(self.client) for empty_field in ("level_of_education", "gender", "country", "state", "bio",): assert response.data[empty_field] is None From 43a0277a399d0980decc73681b09bf17cd2e8df8 Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Wed, 20 Apr 2022 15:30:56 +0000 Subject: [PATCH 11/31] feat!: Use more-trusted IP in rate-limiting Previously, our rate-limiting code trusted the entire `X-Forwarded-For` header, allowing a malicious client to spoof that header and evade rate-limiting. This commit introduces a new module and setting allowing us to make a more conservative choice of IPs. - Create new `openedx.core.djangoapps.util.ip` module for producing the IP "external chain" for requests based on the XFF header and the REMOTE_ADDR. - Include a function that gives the safest choice of IPs. - Add new setting `CLOSEST_CLIENT_IP_FROM_HEADERS` for configuring how the external chain is derived (i.e. setting the trust boundary). Currently has a default, but we may want to make it mandatory in the future. - Change `django-ratelimit` code to use the proximate IP in the external chain -- the one just outside the trust boundary. Also: - Change `XForwardedForMiddleware` to use more conservative choice for its `REMOTE_ADDR` override - Other adjustments to `XForwardedForMiddleware` as needed in order to initialize new module and support code that needs the real `REMOTE_ADDR` value - Metrics for observability into the change (and XFF composition) - Feature switch to restore legacy mode if needed This also gives us a path forward to removing use of the django-ipware package, which is no longer maintained and has a handful of bugs that make it difficult to use safely. Internal ticket: ARCHBOM-2056 Backported from a251d182814a238d06e146878e0eb379189aea77 --- cms/envs/devstack.py | 4 + cms/envs/test.py | 4 + lms/envs/devstack.py | 4 + lms/envs/test.py | 4 + openedx/core/djangoapps/util/ip.py | 479 ++++++++++++++++++ openedx/core/djangoapps/util/ratelimit.py | 15 +- openedx/core/djangoapps/util/tests/test_ip.py | 355 +++++++++++++ .../djangoapps/util/tests/test_ratelimit.py | 48 ++ .../core/lib/x_forwarded_for/middleware.py | 110 +++- .../x_forwarded_for/tests/test_middleware.py | 13 +- 10 files changed, 1005 insertions(+), 31 deletions(-) create mode 100644 openedx/core/djangoapps/util/ip.py create mode 100644 openedx/core/djangoapps/util/tests/test_ip.py create mode 100644 openedx/core/djangoapps/util/tests/test_ratelimit.py diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index a7c08277821..7d8f1431f80 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -261,3 +261,7 @@ def should_show_debug_toolbar(request): # lint-amnesty, pylint: disable=missing # Don't form the return redirect URL with HTTPS on devstack SOCIAL_AUTH_REDIRECT_IS_HTTPS = False + +#################### Network configuration #################### +# Devstack is directly exposed to the caller +CLOSEST_CLIENT_IP_FROM_HEADERS = [] diff --git a/cms/envs/test.py b/cms/envs/test.py index 772be14a259..751af0ff4b3 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -337,3 +337,7 @@ ############### Settings for proctoring ############### PROCTORING_USER_OBFUSCATION_KEY = 'test_key' + +#################### Network configuration #################### +# Tests are not behind any proxies +CLOSEST_CLIENT_IP_FROM_HEADERS = [] diff --git a/lms/envs/devstack.py b/lms/envs/devstack.py index 4df21b4ca60..97e907c1ac0 100644 --- a/lms/envs/devstack.py +++ b/lms/envs/devstack.py @@ -442,3 +442,7 @@ def should_show_debug_toolbar(request): # lint-amnesty, pylint: disable=missing #################### Webpack Configuration Settings ############################## WEBPACK_LOADER['DEFAULT']['TIMEOUT'] = 5 + +#################### Network configuration #################### +# Devstack is directly exposed to the caller +CLOSEST_CLIENT_IP_FROM_HEADERS = [] diff --git a/lms/envs/test.py b/lms/envs/test.py index 8b788d1ef78..69b290fc53e 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -599,3 +599,7 @@ RESET_PASSWORD_API_RATELIMIT = '2/m' CORS_ORIGIN_WHITELIST = ['https://sandbox.edx.org'] + +#################### Network configuration #################### +# Tests are not behind any proxies +CLOSEST_CLIENT_IP_FROM_HEADERS = [] diff --git a/openedx/core/djangoapps/util/ip.py b/openedx/core/djangoapps/util/ip.py new file mode 100644 index 00000000000..c14514dc6fd --- /dev/null +++ b/openedx/core/djangoapps/util/ip.py @@ -0,0 +1,479 @@ +""" +Utilities for determining the IP address of a request. + + +Summary +======= + +For developers: + +- Call ``get_safest_client_ip`` whenever you want to know the caller's IP address +- Make sure ``init_client_ips`` is called as early as possible in the middleware stack +- See the "Guidance for developers" section for more advanced usage + +For site operators: + +- See the "Configuration" section for important information and guidance + +For everyone: + +- Background information is available in the "Concepts" section + + +Concepts +======== + +- The *IP chain* is the list of IPs in the ``X-Forwarded-For`` (XFF) header followed + by the ``REMOTE_ADDR`` value. If all involved parties are telling the truth, this + is the list of IP addresses that have relayed the HTTP request. However, due to + the possibility of spoofing, this raw data cannot be used directly for all + purposes: + + - The rightmost IP in the chain is the IP that has directly connected with the + server and sent or relayed the request. In most deployments, this is likely + to be a reverse proxy such as nginx. In any case it is the "closest" IP (in + the sense of the request chain, not in terms of geographic proximity.) + - The next closest IP, if present, is the one that the closest IP *claims* + sent the request to it. Each IP in the chain can only vouch for the + correctness of the IP immediately to its left in the list. + - In a normal, unspoofed request, the leftmost IP is the "real" client IP, the + IP of the computer that made the original request. + - However, clients can send a fake XFF header, so the leftmost IP in the chain + cannot be trusted in the general case. In fact, the only IP that can be + trusted absolutely is the rightmost one. + - The challenge is to determine what the leftmost *trusted* IP is, as this is + the most accurate we can get without compromising on security. + +- The *external chain* is some prefix of the IP chain that stops before the + (recognized) edge of the deployment's infrastructure. That is, the external + chain is the portion of the IP chain that is to the left of some trust + boundary, as determined by configuration or some fallback method. This is the + list of IPs that can all plausibly be considered the "real" IP of the client. + If the server is configured correctly this may contain, in order: Any IPs + spoofed by the client, the client's own IP, IPs of any forwarding HTTP proxies + specified by the client, and then IPs of any reverse HTTP proxies the + request passed through *before* reaching the deployment's own infrastructure + (CDN, load balancer, etc.) + + - Caveat: In the case where the request is being sent through an anonymizing + proxy such as a VPN, the VPN's exit node IP is considered the "real" client + IP. + - Despite the name, this chain may contain private-range IP addresses, in + particular if a request originates from another server in the same + datacenter. + + +Guidance for developers +======================= + +Almost anywhere you care about IP address, just call ``get_safest_client_ip``. +This will get you the *rightmost* IP of the external chain (defined above). +Because it cannot be easily spoofed by the caller, it is suitable for adversarial +use-cases such as: + +- Rate-limiting +- Only allowing certain IPs to access a resource (or alternatively, blocking them) + +In some less common situations where you need the entire external chain, you +can call ``get_all_client_ips`. This returns a list of IP addresses, although for +the great majority of normal requests this will be a list of length 1. This list is +appropriate for when you're recording IPs for manual review or need to make a +decision based on all of the IPs (no matter which one is the "real" one. This might +include: + +- Audit logs +- Telling a user about other active sessions on their account +- Georestriction + +In some very rare cases you might want just a single IP that isn't rightmost. In +some cases you might ask for the entire external chain and then take the leftmost +IP. This should only be used in non-adversarial situations, and is usually the wrong +choice, but may be appropriate for: + +- Localization (if other HTTP headers aren't sufficient) +- Analytics + + +Configuration +============= + +Configuration is via ``CLOSEST_CLIENT_IP_FROM_HEADERS``, which allows specifying +an HTTP header that will be trusted to report the rightmost IP in the external chain. +See setting annotation for details, but guidance on common configurations is provided +here: + +- If you use a CDN as your outermost proxy: + + - Find what header your CDN sends to its origin that indicates the remote address it + sees on inbound connections. For example, with Cloudflare this is ``CF-Connecting-IP``. + - Ensure that your CDN always overrides this header if it exists in the inbound request, + and never accepts a value provided by the client. Some CDNs are better than others + about this. + - Recommended setting, using Cloudflare as the example:: + + CLOSEST_CLIENT_IP_FROM_HEADERS: + - name: CF-Connecting-IP + index: 0 + + It would be equivalent to use ``-1`` as the index since there is always one and only + one IP in this header, and Python list indexing rules are used here. + - As a general rule, you should also ensure that traffic cannot bypass the CDN and reach + your origin directly, since otherwise attackers will be able to spoof their IP address + (and bypass protections your CDN provides). You may need to arrange for your CDN to set + a header containing a shared secret. + +- If your outermost proxy is an AWS ELB or other proxy on the same local network as your + server, or you have any other configuration in which your proxies and application speak + to each other using private-range IP addresses: + + - You can rely on the rightmost public IP in the IP chain to be the safest client IP. + To do this, set your configuration for zero trusted headers:: + + CLOSEST_CLIENT_IP_FROM_HEADERS: [] + + - This assumes that 1) your outermost proxy always appends to ``X-Forwarded-For``, and + 2) any further proxies between that one and your application either append to it + (ideal) or pass it along unchanged (not ideal, but workable). This is true by default + for most proxy software. + +- If you have any reverse proxy that will be seen by the next proxy or your application as + having a public IP: + + - You'll need to rely on having a consistent *number* of proxies in front of your + application, and you'll need to know which ones append to the ``X-Forwarded-For`` + header instead of just passing it unchanged. + - Once you know the number of your proxies in the chain that append, you can use this + count to say that the Nth-from-last IP in the ``X-Forwarded-For`` is the closest client + IP. For example, if you had two, you would use ``-2`` (note the negative sign) to + indicate the second-from-last IP:: + + CLOSEST_CLIENT_IP_FROM_HEADERS: + - name: X-Forwarded-For + index: -2 + + - This is fragile in the face of network configuration changes, so having your outermost + proxy set a special header is preferred. + - Configuring the proxy count too low will result in rate-limiting your own proxies; + configuring it too high will allow attackers to bypass rate-limiting. + - Side note: Even if you don't use it for ``CLOSEST_CLIENT_IP_FROM_HEADERS``, this + proxy-counting approach will be required for configuring django-rest-framework's + ``NUM_PROXIES`` setting. + +- If your application is directly exposed to the public internet, without even a local proxy: + + - This is an unusual configuration, but simple to configure; with no proxies, just indicate + that there are no trusted headers and therefore the closest public IP should be used:: + + CLOSEST_CLIENT_IP_FROM_HEADERS: [] +""" + +import ipaddress +import warnings + +from django.conf import settings +from edx_toggles.toggles import WaffleSwitch + +# .. toggle_name: ip.legacy +# .. toggle_implementation: WaffleSwitch +# .. toggle_default: False +# .. toggle_description: Emergency switch to revert to use the older, less secure method for +# IP determination. 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. +# .. toggle_warning: This switch does not control the behavior of this module. Callers must +# opt into querying this switch, and can call ``get_legacy_ip`` if the switch is enabled. +# .. toggle_use_cases: temporary +# .. toggle_creation_date: 2022-03-24 +# .. toggle_target_removal_date: 2022-07-01 +# .. toggle_tickets: https://openedx.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']) + + +def _get_meta_ip_strs(request, header_name): + """ + Get a list of IPs from a header in the given request. + + Return the list of IPs the request is carrying on this header, which is + expected to be comma-delimited if it contains more than one. Response + may be an empty list for missing or empty header. List items may not be + valid IPs. + """ + if not header_name: + return [] + + field_name = 'HTTP_' + header_name.replace('-', '_').upper() + header_value = request.META.get(field_name, '').strip() + + if header_value: + return [s.strip() for s in header_value.split(',')] + else: + return [] + + +def get_raw_ip_chain(request): + """ + Retrieve the full IP chain from this request, as list of raw strings. + + This is uninterpreted and unparsed, except for splitting on commas and + removing extraneous whitespace. + """ + return _get_meta_ip_strs(request, 'X-Forwarded-For') + [request.META['REMOTE_ADDR']] + + +def _get_usable_ip_chain(request): + """ + Retrieve the full IP chain from this request, as parsed addresses. + + The IP chain is the X-Forwarded-For header, followed by the REMOTE_ADDR. + This list is then narrowed to the largest suffix that can be parsed as + IP addresses. + """ + parsed = [] + for ip_str in reversed(get_raw_ip_chain(request)): + try: + parsed.append(ipaddress.ip_address(ip_str)) + except ValueError: + break + return list(reversed(parsed)) + + +def _remove_tail(elements, f_discard): + """ + Remove items from the tail of the given list until f_discard returns false. + + - elements is a list + - f_discard is a function that accepts an item from the list and returns + true if it should be discarded from the tail + + Returns a new list that is a possibly-empty prefix of the input list. + + (This is basically itertools.dropwhile on a reversed list.) + """ + prefix = elements[:] + while prefix and f_discard(prefix[-1]): + prefix.pop() + return prefix + + +def _get_client_ips_via_xff(request): + """ + Get the external chain of the request by discarding private IPs. + + This is a strategy used by ``get_all_client_ips`` and should not be used + directly. + + Returns a list of *parsed* IP addresses, one of: + + - A list ending in a publicly routable IP + - A list with a single, private-range IP + - An empty list, if REMOTE_ADDR was unparseable as an IP address. This + would be very unusual but could possibly happen if a local reverse proxy + used a domain socket rather than a TCP connection. + """ + ip_chain = _get_usable_ip_chain(request) + external_chain = _remove_tail(ip_chain, lambda ip: not ip.is_global) + + # If the external_chain is in fact all private, everything will have been + # removed. In that case, just return the leftmost IP it would have + # considered, even though it must be a private IP. + return external_chain or ip_chain[:1] + + +# .. setting_name: CLOSEST_CLIENT_IP_FROM_HEADERS +# .. setting_default: [] +# .. setting_description: A list of header/index pairs to use for determining the IP in the +# IP chain that is just outside of this deployment's infrastructure boundary -- that is, +# the rightmost address in the IP chain that is *not* owned by the deployment. (See module +# docstring for background and definitions, as well as guidance on configuration.) +# Each list entry is a dict containing a header name and an index into that header. This will +# control how the client's IP addresses are determined for attribution, tracking, rate-limiting, +# or other general-purpose needs. +# The named header must contain a list of IP addresses separated by commas, with whitespace +# tolerated around each address. The index is used for a Python list lookup, e.g. 0 is the first +# element and -2 is the second from the end. +# Header/index pairs will be tried in turn until the first one that yields a usable IP, which +# will then be used to determine the end of the external chain. +# If the setting is an empty list, or if none of the entries yields a usable IP (header is +# missing, index out of range, IP not in IP chain), then a fallback strategy will be used +# instead: Private-range IPs will be discarded from the right of the IP chain until a public +# IP is found, or the chain shrinks to one IP. This entry will then be considered the rightmost +# end of the external chain. +# Migrations from one network configuration to another may be accomplished by first adding the +# new header to the list, making the networking change, and then removing the old one. +# .. setting_warnings: Changes to the networking configuration that are not coordinated with +# this setting may allow callers to spoof their IP address. + + +def _get_trusted_header_ip(request, header_name, index): + """ + Read a parsed IP address from a header at the specified position. + + Helper function for ``_get_client_ips_via_trusted_header``. + + Returns None if header is missing, index is out of range, or the located + entry can't be parsed as an IP address. + """ + ip_strs = _get_meta_ip_strs(request, header_name) + + if not ip_strs: + warnings.warn(f"Configured IP address header was missing: {header_name!r}", UserWarning) + return None + + try: + trusted_ip_str = ip_strs[index] + except IndexError: + warnings.warn( + "Configured index into IP address header is out of range: " + f"{header_name!r}:{index!r} " + f"(actual length {len(ip_strs)})", + UserWarning + ) + return None + + try: + return ipaddress.ip_address(trusted_ip_str) + except ValueError: + warnings.warn( + "Configured trusted IP address header contained invalid IP: " + f"{header_name!r}:{index!r}", + UserWarning + ) + + +def _get_client_ips_via_trusted_header(request): + """ + Get the external chain by reading the trust boundary from a header. + + This is a strategy used by ``get_all_client_ips`` and should not be used + directly. It does not implement any fallback in case of misconfiguration. + + Uses ``CLOSEST_CLIENT_IP_FROM_HEADERS`` to identify the IP just outside of + the deployment's infrastructure boundary, and uses the rightmost position + of this to determine where the external chain stops. See setting docs for + more details. + + Returns one of the following: + + - A non-empty list of *parsed* IP addresses, where the rightmost IP is the + same as the one identified in the trusted header. + - None if no headers configured or all headers are unusable. + + A configured header can be unusable if it's missing from the request, the + index is out of range, the indicated entry in the header can't be parsed + as an IP address, or the IP in the header can't be found in the IP chain. + """ + header_entries = getattr(settings, 'CLOSEST_CLIENT_IP_FROM_HEADERS', []) + + full_chain = _get_usable_ip_chain(request) + external_chain = [] + + for entry in header_entries: + header_name = entry['name'] + index = entry['index'] + if closest_client_ip := _get_trusted_header_ip(request, header_name, index): + # The equality check in this predicate is why we use parsed IP + # addresses -- ::1 should compare as equal to 0:0:0:0:0:0:0:1. + external_chain = _remove_tail(full_chain, lambda ip: ip != closest_client_ip) # pylint: disable=cell-var-from-loop + if external_chain: + break + else: + warnings.warn( + f"Ignoring trusted header IP {header_name!r}:{index!r} " + "because it was not found in the actual IP chain.", + UserWarning + ) + + return external_chain + + +def _compute_client_ips(request): + """ + Get the request's external chain, a non-empty list of IP address strings. + + Warning: should only be called once and cached by ``init_client_ips``. + + Prefer to use ``get_all_client_ips`` to retrieve the value stored on the + request, unless you are sure that later middleware has not modified + the REMOTE_ADDR in-place. + + This function will attempt several strategies to determine the external chain: + + - If ``CLOSEST_CLIENT_IP_FROM_HEADERS`` is configured and usable, it will be + used to determine the rightmost end of the external chain (by reading a + trusted HTTP header). + - If that does not yield a result, fall back to assuming that the rightmost + public IP address in the IP chain is the end of the external chain. (For an + in-datacenter HTTP request, may instead yield a list with a private IP.) + """ + # In practice the fallback to REMOTE_ADDR should never happen, since that + # would require that value to be present and malformed but with no XFF + # present. + ips = _get_client_ips_via_trusted_header(request) \ + or _get_client_ips_via_xff(request) \ + or [request.META['REMOTE_ADDR']] + + return [str(ip) for ip in ips] + + +def init_client_ips(request): + """ + Compute the request's external chain and store it in the request. + + This should be called early in the middleware stack in order to avoid + being called after another middleware that overwrites ``REMOTE_ADDR``, + which is a pattern some apps use. + + If called multiple times or if ``CLIENT_IPS`` is already present in + ``request.META``, will just warn. + """ + if 'CLIENT_IPS' in request.META: + warnings.warn("init_client_ips refusing to overwrite existing CLIENT_IPS") + else: + request.META['CLIENT_IPS'] = _compute_client_ips(request) + + +def get_all_client_ips(request): + """ + Get the request's external chain, a non-empty list of IP address strings. + + Most consumers of IP addresses should just use ``get_safest_client_ip``. + + Calls ``init_client_ips`` if needed. + """ + if 'CLIENT_IPS' not in request.META: + init_client_ips(request) + + return request.META['CLIENT_IPS'] + + +def get_safest_client_ip(request): + """ + Get the safest choice of client IP. + + Returns a single string containing the IP address that most likely + represents the originator of the HTTP call, without compromising on + safety. + + This is always the rightmost value in the external IP chain that + is returned by ``get_all_client_ips``. See module docstring for + more details. + """ + return get_all_client_ips(request)[-1] diff --git a/openedx/core/djangoapps/util/ratelimit.py b/openedx/core/djangoapps/util/ratelimit.py index 6fd7266f559..ffcdd86f531 100644 --- a/openedx/core/djangoapps/util/ratelimit.py +++ b/openedx/core/djangoapps/util/ratelimit.py @@ -3,11 +3,22 @@ """ from uuid import uuid4 -from ipware.ip import get_client_ip +from openedx.core.djangoapps.util import ip def real_ip(group, request): # pylint: disable=unused-argument - return get_client_ip(request)[0] + """ + Get a client IP suitable for use in rate-limiting. + + To prevent evasion of rate-limiting, use the safest (rightmost) IP in the + external IP chain. + + (Intended to be called by ``django-ratelimit``, hence the unused argument.) + """ + if ip.USE_LEGACY_IP.is_enabled(): + return ip.get_legacy_ip(request) + else: + 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_ip.py b/openedx/core/djangoapps/util/tests/test_ip.py new file mode 100644 index 00000000000..b717ca56703 --- /dev/null +++ b/openedx/core/djangoapps/util/tests/test_ip.py @@ -0,0 +1,355 @@ +""" +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 warnings +from contextlib import contextmanager + +import ddt +from django.test import TestCase +from django.test.client import RequestFactory +from django.test.utils import override_settings + +import openedx.core.djangoapps.util.ip as ip +from openedx.core.lib.x_forwarded_for.middleware import XForwardedForMiddleware + + +@contextmanager +def warning_messages(): + """ + Context manager which produces a list of warning messages as the context + value (only populated after block ends). + """ + with warnings.catch_warnings(record=True) as caught_warnings: + warnings.simplefilter('always') + messages = [] + yield messages + # Converted to message strings for easier debugging + messages.extend(str(w.message) for w in caught_warnings) + + +@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 ip.get_legacy_ip(self.request) == expected + + # Check that it still works after the XFF middleware has done its dirty work + XForwardedForMiddleware().process_request(self.request) + assert ip.get_legacy_ip(self.request) == expected + + @ddt.unpack + @ddt.data( + ({}, 'Some-Thing', []), + ({'HTTP_SOME_THING': 'stuff'}, 'Some-Thing', ['stuff']), + ({'HTTP_SOME_THING': 'stuff'}, 'Some-Thing', ['stuff']), + ({'HTTP_SOME_THING': ' so,much , stuff '}, 'Some-Thing', ['so', 'much', 'stuff']), + ) + def test_get_meta_ip_strs(self, add_meta, header_name, expected): + self.request.META.update(add_meta) + assert ip._get_meta_ip_strs(self.request, header_name) == expected # pylint: disable=protected-access + + @ddt.unpack + @ddt.data( + # Form the IP chain and parse it (notice the IPv6 is canonicalized) + ( + {'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', '1.2.3.4', '10.0.3.0', '::1'], + ), + + # XFF is not required + ({'REMOTE_ADDR': '127.0.0.2'}, ['127.0.0.2']), + + # Strips off junk and anything to the left of it + ( + { + 'HTTP_X_FORWARDED_FOR': '6.6.6.6, XXXXXXXXX, 7.8.9.0, XXXXXXXXX, 1.2.3.4', + 'REMOTE_ADDR': '10.0.3.0' + }, + ['1.2.3.4', '10.0.3.0'], + ), + ) + def test_get_usable_ip_chain(self, request_meta, expected_strs): + self.request.META = request_meta + actual_ips = ip._get_usable_ip_chain(self.request) # pylint: disable=protected-access + assert [str(ip) for ip in actual_ips] == expected_strs + + @ddt.unpack + @ddt.data( + # Empty returns empty + ([], lambda x: False, []), + ([], lambda x: True, []), + + # Can return whole list or remove all of it + ([1, 2, 3, 4], lambda x: False, [1, 2, 3, 4]), + ([1, 2, 3, 4], lambda x: True, []), + + # Walks from right, stops on first false, keeps that element + ([2, 0, 1, 0, 3, 5], lambda x: x != 0, [2, 0, 1, 0]), + ) + def test_remove_tail(self, elements, f_discard, expected): + assert ip._remove_tail(elements, f_discard) == expected # pylint: disable=protected-access + + @ddt.unpack + @ddt.data( + # Walk from right, dropping private-range IPs + ('7:8:9:0::, 1.2.3.4, 10.0.3.0', '::1', ['7:8:9::', '1.2.3.4']), + ('7.8.9.0', '1.2.3.4', ['7.8.9.0', '1.2.3.4']), + + # Publicly exposed server (no XFF added by proxies) + (None, '1.2.3.4', ['1.2.3.4']), + + # If XFF is missing, just accept a private IP (maybe an inter-service call + # from another server in the same datacenter) + (None, '127.0.0.2', ['127.0.0.2']), + + # If we reach a public IP, don't worry about junk farther on + ('XXXXXXXXX, 1:2:3:4::', '10.0.0.1', ['1:2:3:4::']), + + # If we find junk or we run out of IPs before finding a public + # one, the best we can do is a private IP. (Should never + # happen for a public IP, but here for completeness.) + ('7.8.9.0, XXXXXXXXX, 10.0.3.0', '127.0.0.2', ['10.0.3.0']), + (None, '::1', ['::1']), + + # Nothing usable (again, should never happen) + (None, '', []), + (None, 'XXXXXXXXX', []), + ('1.2.3.4', 'XXXXXXXXX', []), + ) + def test_get_client_ips_via_xff(self, xff, remote_addr, expected_strs): + request_meta = {'REMOTE_ADDR': remote_addr, 'HTTP_X_FORWARDED_FOR': xff} + request_meta = {k: v for k, v in request_meta.items() if v is not None} + self.request.META = request_meta + + assert [str(ip) for ip in ip._get_client_ips_via_xff(self.request)] == expected_strs # pylint: disable=protected-access + + @ddt.unpack + @ddt.data( + # Happy path + ('Some-Thing', 0, {'HTTP_SOME_THING': '1.2.3.4'}, '1.2.3.4', None), + ('some-thing', -1, {'HTTP_SOME_THING': '1:2:3:4::, 0:0::1'}, '::1', None), + + # Warning: Header present, index out of range + ('Some-Thing', 1, {'HTTP_SOME_THING': '1.2.3.4'}, None, "out of range"), + ('Some-Thing', -2, {'HTTP_SOME_THING': '1.2.3.4'}, None, "out of range"), + + # Warning: Header missing entirely + ('Some-Thing', 0, {}, None, "missing"), + + # Warning: Bad IP address + ('Some-Thing', 0, {'HTTP_SOME_THING': 'XXXXXXXXX'}, None, "invalid IP"), + ) + def test_get_trusted_header_ip(self, header_name, index, add_meta, expected, warning_substr): + self.request.META.update(add_meta) + + with warning_messages() as caught_warnings: + actual = ip._get_trusted_header_ip(self.request, header_name, index) # pylint: disable=protected-access + + if expected is None: + assert actual is None + else: + assert str(actual) == expected # Stringify again for comparison + + if warning_substr is None: + assert len(caught_warnings) == 0 + else: + assert len(caught_warnings) == 1 + assert warning_substr in caught_warnings[0] + + @ddt.unpack + @ddt.data( + # Most common case: One header in config, and header does exist + ( + [{'name': 'CF-Connecting-IP', 'index': 0}], + { + 'HTTP_X_FORWARDED_FOR': '1.2.3.4', + 'REMOTE_ADDR': '10.0.3.0', + 'HTTP_CF_CONNECTING_IP': '1.2.3.4', + }, + ['1.2.3.4'], + 0, + ), + # More complicated version with intervening proxies and a spoofed IP + ( + [{'name': 'CF-Connecting-IP', 'index': 0}], + { + 'HTTP_X_FORWARDED_FOR': '7.8.9.0, 1.2.3.4, 5.5.5.5, 10.0.3.0', + 'REMOTE_ADDR': '127.0.0.2', + 'HTTP_CF_CONNECTING_IP': ' 1.2.3.4 ', # tests that whitespace is stripped + }, + ['7.8.9.0', '1.2.3.4'], + 0, + ), + + # Uses *rightmost* position of identified IP if multiple are present + # (prevent spoofing when client calling through a trustworthy proxy) + ( + [{'name': 'CF-Connecting-IP', 'index': 0}], + { + 'HTTP_X_FORWARDED_FOR': '6.6.6.6, 1.2.3.4, 7.8.9.0, 1.2.3.4, 5.5.5.5', + 'REMOTE_ADDR': '10.0.3.0', + 'HTTP_CF_CONNECTING_IP': '1.2.3.4', + }, + ['6.6.6.6', '1.2.3.4', '7.8.9.0', '1.2.3.4'], + 0, + ), + + # No config? Empty list. + ([], {'REMOTE_ADDR': '1.2.3.4'}, [], 0), + (None, {'REMOTE_ADDR': '1.2.3.4'}, [], 0), + + # One lookup failure (a warning) before finding a usable header + ( + [ + {'name': 'X-Real-IP', 'index': 0}, + {'name': 'CF-Connecting-IP', 'index': 0}, + ], + { + 'HTTP_X_FORWARDED_FOR': '7.8.9.0, 1.2.3.4, 5.5.5.5', + 'REMOTE_ADDR': '10.0.3.0', + 'HTTP_CF_CONNECTING_IP': '1.2.3.4', + }, + ['7.8.9.0', '1.2.3.4'], + 1, + ), + + # Configured, but none of the headers are present + ( + [ + {'name': 'CF-Connecting-IP', 'index': 0}, + {'name': 'X-Forwarded-For', 'index': -2}, + ], + {'REMOTE_ADDR': '1.2.3.4'}, + [], + 2, + ), + + # Can configure to use far end of XFF if needed for some reason + ( + [ + {'name': 'X-Forwarded-For', 'index': 0}, + ], + { + 'HTTP_X_FORWARDED_FOR': '1.2.3.4, 5.5.5.5', + 'REMOTE_ADDR': '10.0.3.0', + }, + ['1.2.3.4'], + 0, + ), + ) + def test_get_client_ips_via_trusted_header(self, cnf_headers, add_meta, expected, warning_count): + self.request.META.update(add_meta) + if cnf_headers is None: + overrides = {} + else: + overrides = {'CLOSEST_CLIENT_IP_FROM_HEADERS': cnf_headers} + + with override_settings(**overrides): + with warning_messages() as caught_warnings: + actual = ip._get_client_ips_via_trusted_header(self.request) # pylint: disable=protected-access + + assert [str(ip) for ip in actual] == expected + assert len(caught_warnings) == warning_count + + @ddt.unpack + @ddt.data( + # Using headers setting + ( + [{'name': 'CF-Connecting-IP', 'index': 0}], + { + 'HTTP_X_FORWARDED_FOR': '7.8.9.0, 1.2.3.4, 5.5.5.5, 10.0.3.0', + 'HTTP_CF_CONNECTING_IP': '1.2.3.4', + 'REMOTE_ADDR': '127.0.0.2', + }, + ['7.8.9.0', '1.2.3.4'], + 0, + ), + + # Fall back when all override headers are unusable, with warnings + ( + [ + {'name': 'CF-Connecting-IP', 'index': 0}, # not actually passed in this request + {'name': 'X-Real-IP', 'index': 2}, # index out of range, so unusable + ], + { + 'HTTP_X_FORWARDED_FOR': '7.8.9.0, 1.2.3.4, 10.0.3.0, 127.0.0.2', + 'HTTP_X_REAL_IP': '10.0.3.0', + 'REMOTE_ADDR': '127.0.0.2', + }, + ['7.8.9.0', '1.2.3.4'], + 2, + ), + + # By default, with the least possible information, do something useful. + # (And no warnings when no override headers are specified.) + ([], {'REMOTE_ADDR': '127.0.0.2'}, ['127.0.0.2'], 0), + ) + def test_compute_client_ips(self, cnf_headers, add_meta, expected, expect_warnings): + """ + Just a few tests to confirm that the correct branch is taken, basically. + """ + if cnf_headers is None: + overrides = {} + else: + overrides = {'CLOSEST_CLIENT_IP_FROM_HEADERS': cnf_headers} + + self.request.META.update(add_meta) + + with override_settings(**overrides): + with warning_messages() as caught_warnings: + actual = ip._compute_client_ips(self.request) # pylint: disable=protected-access + + assert actual == expected + assert len(caught_warnings) == expect_warnings + + def test_get_all_client_ips(self): + """ + Test getter for all IPs. + """ + self.request.META.update({ + 'HTTP_X_FORWARDED_FOR': '7.8.9.0, 1.2.3.4', + 'REMOTE_ADDR': '127.0.0.2', + }) + + assert ip.get_all_client_ips(self.request) == ['7.8.9.0', '1.2.3.4'] + + def test_get_safest_client_ip(self): + """ + Test convenience wrapper for rightmost IP. + """ + self.request.META.update({ + 'HTTP_X_FORWARDED_FOR': '7.8.9.0', + 'REMOTE_ADDR': '1.2.3.4', + }) + + assert ip.get_safest_client_ip(self.request) == '1.2.3.4' diff --git a/openedx/core/djangoapps/util/tests/test_ratelimit.py b/openedx/core/djangoapps/util/tests/test_ratelimit.py new file mode 100644 index 00000000000..35be364ba12 --- /dev/null +++ b/openedx/core/djangoapps/util/tests/test_ratelimit.py @@ -0,0 +1,48 @@ +""" +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.lib.x_forwarded_for.middleware import XForwardedForMiddleware + + +@ddt.ddt +class TestRateLimiting(TestCase): + """Tests for rate limiting and helpers.""" + def setUp(self): + super().setUp() + self.request = RequestFactory().get('/somewhere') + self.request.META.update({ + 'HTTP_X_FORWARDED_FOR': '7.8.9.0, 1.2.3.4, 10.0.3.0', + 'REMOTE_ADDR': '127.0.0.2', + }) + + def test_real_ip(self): + """ + Bare test, no middleware to init the external chain. + """ + assert ratelimit.real_ip(None, self.request) == '1.2.3.4' + + def test_real_ip_after_xff_middleware(self): + """ + More realistic test since XFF middleware meddles with REMOTE_ADDR. + """ + XForwardedForMiddleware().process_request(self.request) + assert ratelimit.real_ip(None, self.request) == '1.2.3.4' + + @override_waffle_switch(ratelimit.ip.USE_LEGACY_IP, True) + def test_legacy_switch(self): + assert ratelimit.real_ip(None, self.request) == '7.8.9.0' + + @override_waffle_switch(ratelimit.ip.USE_LEGACY_IP, True) + def test_legacy_switch_after_xff_middleware(self): + """ + Again, but with XFF Middleware running first. + """ + XForwardedForMiddleware().process_request(self.request) + assert ratelimit.real_ip(None, self.request) == '7.8.9.0' diff --git a/openedx/core/lib/x_forwarded_for/middleware.py b/openedx/core/lib/x_forwarded_for/middleware.py index 162a6e3d21b..4a9d287dc91 100644 --- a/openedx/core/lib/x_forwarded_for/middleware.py +++ b/openedx/core/lib/x_forwarded_for/middleware.py @@ -1,39 +1,103 @@ """ -Middleware to use the X-Forwarded-For header as the request IP. -Updated the libray to use HTTP_HOST and X-Forwarded-Port as -SERVER_NAME and SERVER_PORT. +Middleware to adjust some request values to account for reverse proxies. """ +import ipaddress +import warnings from django.utils.deprecation import MiddlewareMixin from edx_django_utils.monitoring import set_custom_attribute +import openedx.core.djangoapps.util.ip as ip -class XForwardedForMiddleware(MiddlewareMixin): + +def _ip_type(ip_str): + """ + Produce a short, approximate term describing the IP address type. """ - Gunicorn 19.0 has breaking changes for REMOTE_ADDR, SERVER_* headers - that can not override with forwarded and host headers. - This middleware can be used to update these headers set by proxy configuration. + try: + if ipaddress.ip_address(ip_str).is_global: + # Globally routable IPs are what most people think of as + # "public" IPs. + return 'pub' + else: + # Anything else is "private", although it may actually be + # link-local, multicast, etc. rather than a private-range + # IP, per se. + return 'priv' + except ValueError: + # Something unparseable. + return 'unknown' + +class XForwardedForMiddleware(MiddlewareMixin): + """ + The middleware name is outdated, since it no longer uses a hardcoded reference + to X-Forwarded-For. """ - def process_request(self, request): # lint-amnesty, pylint: disable=useless-return + def process_request(self, request): """ - Process the given request, update the value of REMOTE_ADDR, SERVER_NAME and SERVER_PORT based - on X-Forwarded-For, HTTP_HOST and X-Forwarded-Port headers + Process the given request, update the value of SERVER_NAME and SERVER_PORT based + on HTTP_HOST and X-Forwarded-Port headers that were set by some upstream proxy. """ - # Give some observability into X-Forwarded-For length. Useful - # for monitoring in case of unexpected changes, etc. - xff = request.META.get('HTTP_X_FORWARDED_FOR') - xff_len = xff.count(',') + 1 if xff else 0 - # IP chain is XFF + REMOTE_ADDR - set_custom_attribute('ip_chain.count', xff_len + 1) - - for field, header in [("HTTP_X_FORWARDED_FOR", "REMOTE_ADDR"), ("HTTP_HOST", "SERVER_NAME"), + # Must be called before the REMOTE_ADDR override that happens below. + # 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'] + + try: + # Give some observability into IP chain length and composition. Useful + # for monitoring in case of unexpected network config changes, etc. + ip_chain = ip.get_raw_ip_chain(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', 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)) + except BaseException: + warnings.warn('Error while computing IP chain metrics') + + # Older code for the Gunicorn 19.0 upgrade. Original docstring: + # + # Gunicorn 19.0 has breaking changes for REMOTE_ADDR, SERVER_* headers + # that can not override with forwarded and host headers. + # This middleware can be used to update these headers set by proxy configuration. + # + # REMOTE_ADDR has since been removed from this override and is now + # handled separately below. + for field, header in [("HTTP_HOST", "SERVER_NAME"), ("HTTP_X_FORWARDED_PORT", "SERVER_PORT")]: if field in request.META: - if ',' in request.META[field]: - request.META[header] = request.META[field].split(",")[0].strip() - else: - request.META[header] = request.META[field] + request.META[header] = request.META[field] - return None + # This should eventually be deleted, but is here to avoid breaking + # older code. This override was previously implemented in the above + # code by using the first IP in X-Forwarded-For, and just overwrote + # REMOTE_ADDR, which probably creates more problems elsewhere (as + # the full IP chain is then no longer possible to construct.) + # + # The old code chose the leftmost IP in the external chain + # (first in XFF) but now chooses the rightmost, which is the + # safer choice when the specific needs of the relying code are + # not known. + # + # Any code that is relying on this override should instead be + # using the IP utility module used here, which is configurable and + # 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 + # 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 ip.USE_LEGACY_IP.is_enabled(): + request.META['REMOTE_ADDR'] = ip.get_legacy_ip(request) + else: + request.META['REMOTE_ADDR'] = ip.get_safest_client_ip(request) diff --git a/openedx/core/lib/x_forwarded_for/tests/test_middleware.py b/openedx/core/lib/x_forwarded_for/tests/test_middleware.py index a3b53c17c7e..492c29a107f 100644 --- a/openedx/core/lib/x_forwarded_for/tests/test_middleware.py +++ b/openedx/core/lib/x_forwarded_for/tests/test_middleware.py @@ -39,11 +39,11 @@ class TestXForwardedForMiddleware(TestCase): }, ), - # REMOTE_ADDR can also be overridden + # REMOTE_ADDR can also be overridden (chooses rightmost) ( {'HTTP_X_FORWARDED_FOR': '7.8.9.0, 1.2.3.4'}, { - 'REMOTE_ADDR': '7.8.9.0', + 'REMOTE_ADDR': '1.2.3.4', }, ), ) @@ -60,12 +60,12 @@ def test_overrides(self, add_meta, expected_meta_include): @ddt.unpack @ddt.data( - (None, 1), - ('1.2.3.4', 2), - ('XXXXXXXX, 1.2.3.4, 5.5.5.5', 4), + (None, 1, 'priv'), + ('1.2.3.4', 2, 'pub-priv'), + ('XXXXXXXX, 1.2.3.4, 5.5.5.5', 4, 'unknown-pub-pub-priv'), ) @patch("openedx.core.lib.x_forwarded_for.middleware.set_custom_attribute") - def test_xff_metrics(self, xff, expected_count, mock_set_custom_attribute): + def test_xff_metrics(self, xff, expected_count, expected_types, mock_set_custom_attribute): request = RequestFactory().get('/somewhere') if xff is not None: request.META['HTTP_X_FORWARDED_FOR'] = xff @@ -74,4 +74,5 @@ def test_xff_metrics(self, xff, expected_count, mock_set_custom_attribute): mock_set_custom_attribute.assert_has_calls([ call('ip_chain.count', expected_count), + call('ip_chain.types', expected_types), ]) From 2bbf06e48ca390cce225d4cf607719d5a026ec05 Mon Sep 17 00:00:00 2001 From: Ned Batchelder Date: Tue, 8 Feb 2022 11:21:34 -0500 Subject: [PATCH 12/31] build: the Django matrix needs to use the pinned version also. The logic here seems to work, but Django 4.0 won't install over our other pinned requirements, so tests fail for Django 4.0. (cherry picked from commit e7caec5ab807fe8bb42a7f1494559dc2e0207023) --- .github/workflows/unit-tests.yml | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 9c27711d5a7..438ab7c8e9c 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -13,7 +13,9 @@ jobs: strategy: matrix: python-version: ['3.8'] - django-version: ["3.2"] + django-version: + - "pinned" + #- "4.0" shard_name: [ "lms-1", "lms-2", @@ -57,7 +59,15 @@ jobs: run: | sudo pip install -r requirements/pip.txt sudo pip install -r requirements/edx/testing.txt - sudo pip install "django~=${{ matrix.django-version }}.0" + if [[ "${{ matrix.django-version }}" == "pinned" ]]; then + sudo pip install -r requirements/edx/django.txt + else + sudo pip install "django~=${{ matrix.django-version }}.0" + fi + + - name: list installed package versions + run: | + sudo pip freeze - name: get unit tests for shard run: | From abe5c63e2c24e570ca5ee3a6b065bcb427849337 Mon Sep 17 00:00:00 2001 From: ha-D Date: Thu, 26 Aug 2021 23:13:34 +0000 Subject: [PATCH 13/31] fixup! feat: options for excluding courses from search --- cms/djangoapps/contentstore/courseware_index.py | 2 ++ lms/envs/common.py | 17 ++++++++++++++++- lms/envs/production.py | 9 +++++++++ .../courseware_search/lms_filter_generator.py | 6 ++++++ 4 files changed, 33 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/courseware_index.py b/cms/djangoapps/contentstore/courseware_index.py index 5d9b627ec02..1878e04b3b0 100644 --- a/cms/djangoapps/contentstore/courseware_index.py +++ b/cms/djangoapps/contentstore/courseware_index.py @@ -587,6 +587,8 @@ class CourseAboutSearchIndexer(CoursewareSearchIndexer): AboutInfo("org", AboutInfo.PROPERTY, AboutInfo.FROM_COURSE_PROPERTY), AboutInfo("modes", AboutInfo.PROPERTY, AboutInfo.FROM_COURSE_MODE), AboutInfo("language", AboutInfo.PROPERTY, AboutInfo.FROM_COURSE_PROPERTY), + AboutInfo("invitation_only", AboutInfo.PROPERTY, AboutInfo.FROM_COURSE_PROPERTY), + AboutInfo("catalog_visibility", AboutInfo.PROPERTY, AboutInfo.FROM_COURSE_PROPERTY), ] @classmethod diff --git a/lms/envs/common.py b/lms/envs/common.py index 958d06ccbfd..f0e29f4bbd3 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -817,7 +817,7 @@ # .. toggle_tickets: https://openedx.atlassian.net/browse/OSPR-1880 'ENABLE_HTML_XBLOCK_STUDENT_VIEW_DATA': False, - # .. toggle_name: FEATURES['ENABLE_CHANGE_USER_PASSWORD_ADMIN'] + # .. toggle_name: FEATURES['ENABLE_PASSWORD_RESET_FAILURE_EMAIL'] # .. toggle_implementation: DjangoSetting # .. toggle_default: False # .. toggle_description: Whether to send an email for failed password reset attempts or not. This happens when a @@ -3982,6 +3982,21 @@ def _make_locale_paths(settings): # pylint: disable=missing-function-docstring SEARCH_FILTER_GENERATOR = "lms.lib.courseware_search.lms_filter_generator.LmsSearchFilterGenerator" # Override to skip enrollment start date filtering in course search SEARCH_SKIP_ENROLLMENT_START_DATE_FILTERING = False +# .. toggle_name: SEARCH_SKIP_INVITATION_ONLY_FILTERING +# .. toggle_implementation: DjangoSetting +# .. toggle_default: True +# .. toggle_description: If enabled, invitation-only courses will appear in search results. +# .. toggle_use_cases: open_edx +# .. toggle_creation_date: 2021-08-27 +SEARCH_SKIP_INVITATION_ONLY_FILTERING = True +# .. toggle_name: SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING +# .. toggle_implementation: DjangoSetting +# .. toggle_default: True +# .. toggle_description: If enabled, courses with a catalog_visibility set to "none" will still +# appear in search results. +# .. toggle_use_cases: open_edx +# .. toggle_creation_date: 2021-08-27 +SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING = True # The configuration visibility of account fields. ACCOUNT_VISIBILITY_CONFIGURATION = { diff --git a/lms/envs/production.py b/lms/envs/production.py index d517908c060..89cb7f2b419 100644 --- a/lms/envs/production.py +++ b/lms/envs/production.py @@ -729,6 +729,15 @@ def get_env_setting(setting): SEARCH_ENGINE = "search.elastic.ElasticSearchEngine" SEARCH_FILTER_GENERATOR = ENV_TOKENS.get('SEARCH_FILTER_GENERATOR', SEARCH_FILTER_GENERATOR) +SEARCH_SKIP_INVITATION_ONLY_FILTERING = ENV_TOKENS.get( + 'SEARCH_SKIP_INVITATION_ONLY_FILTERING', + SEARCH_SKIP_INVITATION_ONLY_FILTERING, +) +SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING = ENV_TOKENS.get( + 'SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING', + SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING, +) + # TODO: Once we have successfully upgraded to ES7, switch this back to ELASTIC_SEARCH_CONFIG. ELASTIC_SEARCH_CONFIG = ENV_TOKENS.get('ELASTIC_SEARCH_CONFIG_ES7', [{}]) diff --git a/lms/lib/courseware_search/lms_filter_generator.py b/lms/lib/courseware_search/lms_filter_generator.py index 14b539b4291..c4e5ab7ac73 100644 --- a/lms/lib/courseware_search/lms_filter_generator.py +++ b/lms/lib/courseware_search/lms_filter_generator.py @@ -2,6 +2,7 @@ This file contains implementation override of SearchFilterGenerator which will allow * Filter by all courses in which the user is enrolled in """ +from django.conf import settings from search.filter_generator import SearchFilterGenerator from openedx.core.djangoapps.course_groups.partition_scheme import CohortPartitionScheme @@ -52,4 +53,9 @@ def exclude_dictionary(self, **kwargs): if org_filter_out_set: exclude_dictionary['org'] = list(org_filter_out_set) + if not getattr(settings, "SEARCH_SKIP_INVITATION_ONLY_FILTERING", True): + exclude_dictionary['invitation_only'] = True + if not getattr(settings, "SEARCH_SKIP_SHOW_IN_CATALOG_FILTERING", True): + exclude_dictionary['catalog_visibility'] = 'none' + return exclude_dictionary From df63cbb323c94a3f84f235a299fd29f00d942931 Mon Sep 17 00:00:00 2001 From: Alex Bender Date: Mon, 19 Jul 2021 13:02:37 +0300 Subject: [PATCH 14/31] fix: import+export management commands --- .../management/commands/export_content_library.py | 2 +- .../management/commands/import_content_library.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/management/commands/export_content_library.py b/cms/djangoapps/contentstore/management/commands/export_content_library.py index c07526b35fd..5a53a17ec1d 100644 --- a/cms/djangoapps/contentstore/management/commands/export_content_library.py +++ b/cms/djangoapps/contentstore/management/commands/export_content_library.py @@ -61,6 +61,6 @@ def handle(self, *args, **options): filename = prefix + suffix target = os.path.join(dest_path, filename) tarball.file.seek(0) - with open(target, 'w') as f: + with open(target, 'wb') as f: shutil.copyfileobj(tarball.file, f) print(f'Library "{library.location.library_key}" exported to "{target}"') diff --git a/cms/djangoapps/contentstore/management/commands/import_content_library.py b/cms/djangoapps/contentstore/management/commands/import_content_library.py index 9d33523cdb5..9accd84a86d 100644 --- a/cms/djangoapps/contentstore/management/commands/import_content_library.py +++ b/cms/djangoapps/contentstore/management/commands/import_content_library.py @@ -43,13 +43,13 @@ def handle(self, *args, **options): username = options['owner_username'] data_root = Path(settings.GITHUB_REPO_ROOT) - subdir = base64.urlsafe_b64encode(os.path.basename(archive_path)) + subdir = base64.urlsafe_b64encode(os.path.basename(archive_path).encode('utf-8')).decode('utf-8') course_dir = data_root / subdir # Extract library archive tar_file = tarfile.open(archive_path) # lint-amnesty, pylint: disable=consider-using-with try: - safetar_extractall(tar_file, course_dir.encode('utf-8')) + safetar_extractall(tar_file, course_dir) except SuspiciousOperation as exc: raise CommandError(f'\n=== Course import {archive_path}: Unsafe tar file - {exc.args[0]}\n') # lint-amnesty, pylint: disable=raise-missing-from finally: From 36b0a19f24f3fbae4bb7ef8c8dcce2e64c65b165 Mon Sep 17 00:00:00 2001 From: Demid Date: Fri, 22 Apr 2022 16:24:23 +0300 Subject: [PATCH 15/31] feat: Implement feature flag to disable students un-enrollment (#29326) Implements a feature flag DISABLE_UNENROLLMENT that is used to disable students un-enrollment for all courses. The Unenrollment option should be disabled when this feature is set to True. ref: BB-4951 Co-authored-by: tinumide Co-authored-by: Tim McCormack (cherry picked from commit da4a6d6103df7f76565f77152ac3f4518f90c357) --- cms/envs/common.py | 12 ++++++++++++ .../djangoapps/student/tests/test_enrollment.py | 16 ++++++++++++++++ common/djangoapps/student/views/dashboard.py | 5 +++++ common/djangoapps/student/views/management.py | 6 ++++++ lms/envs/common.py | 12 ++++++++++++ lms/templates/dashboard.html | 7 ++++++- .../dashboard/_dashboard_course_listing.html | 4 ++-- 7 files changed, 59 insertions(+), 3 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index 974e99370a2..13c1375a70f 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -472,6 +472,18 @@ # .. toggle_target_removal_date: 2021-10-01 # .. toggle_tickets: 'https://openedx.atlassian.net/browse/MICROBA-1405' 'ENABLE_V2_CERT_DISPLAY_SETTINGS': False, + + # .. toggle_name: FEATURES['DISABLE_UNENROLLMENT'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. toggle_description: Set to True to disable self-unenrollments via REST API. + # This also hides the "Unenroll" button on the Learner Dashboard. + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2021-10-11 + # .. toggle_warnings: For consistency in user experience, keep the value in sync with the setting of the same name + # in the LMS and CMS. + # .. toggle_tickets: 'https://github.com/open-craft/edx-platform/pull/429' + 'DISABLE_UNENROLLMENT': False, } ENABLE_JASMINE = False diff --git a/common/djangoapps/student/tests/test_enrollment.py b/common/djangoapps/student/tests/test_enrollment.py index 5165ca57d2e..e22d1c9e973 100644 --- a/common/djangoapps/student/tests/test_enrollment.py +++ b/common/djangoapps/student/tests/test_enrollment.py @@ -358,6 +358,22 @@ def test_with_invalid_course_id(self): resp = self._change_enrollment('unenroll', course_id="edx/") assert resp.status_code == 400 + @patch.dict(settings.FEATURES, {'DISABLE_UNENROLLMENT': True}) + def test_unenroll_when_unenrollment_disabled(self): + """ + Tests that a user cannot unenroll when unenrollment has been disabled. + """ + # Enroll the student in the course + CourseEnrollment.enroll(self.user, self.course.id, mode="honor") + + # Attempt to unenroll + resp = self._change_enrollment('unenroll') + assert resp.status_code == 400 + + # Verify that user is still enrolled + is_enrolled = CourseEnrollment.is_enrolled(self.user, self.course.id) + assert is_enrolled + def test_enrollment_limit(self): """ Assert that in a course with max student limit set to 1, we can enroll staff and instructor along with diff --git a/common/djangoapps/student/views/dashboard.py b/common/djangoapps/student/views/dashboard.py index e78b34c7eaf..45252e5abc8 100644 --- a/common/djangoapps/student/views/dashboard.py +++ b/common/djangoapps/student/views/dashboard.py @@ -514,6 +514,10 @@ def student_dashboard(request): # lint-amnesty, pylint: disable=too-many-statem empty_dashboard_message = configuration_helpers.get_value( 'EMPTY_DASHBOARD_MESSAGE', None ) + disable_unenrollment = configuration_helpers.get_value( + 'DISABLE_UNENROLLMENT', + settings.FEATURES.get('DISABLE_UNENROLLMENT') + ) disable_course_limit = request and 'course_limit' in request.GET course_limit = get_dashboard_course_limit() if not disable_course_limit else None @@ -797,6 +801,7 @@ def student_dashboard(request): # lint-amnesty, pylint: disable=too-many-statem # TODO START: clean up as part of REVEM-199 (START) 'course_info': get_dashboard_course_info(user, course_enrollments), # TODO START: clean up as part of REVEM-199 (END) + 'disable_unenrollment': disable_unenrollment, } # Include enterprise learner portal metadata and messaging diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index 9c1768a2f32..5c78d521cfd 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -397,6 +397,12 @@ def change_enrollment(request, check_access=True): # Otherwise, there is only one mode available (the default) return HttpResponse() elif action == "unenroll": + if configuration_helpers.get_value( + "DISABLE_UNENROLLMENT", + settings.FEATURES.get("DISABLE_UNENROLLMENT") + ): + return HttpResponseBadRequest(_("Unenrollment is currently disabled")) + enrollment = CourseEnrollment.get_enrollment(user, course_id) if not enrollment: return HttpResponseBadRequest(_("You are not enrolled in this course")) diff --git a/lms/envs/common.py b/lms/envs/common.py index 958d06ccbfd..bfd5df27bdd 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -948,6 +948,18 @@ # .. toggle_target_removal_date: 2021-10-01 # .. toggle_tickets: 'https://openedx.atlassian.net/browse/MICROBA-1405' 'ENABLE_V2_CERT_DISPLAY_SETTINGS': False, + + # .. toggle_name: FEATURES['DISABLE_UNENROLLMENT'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. toggle_description: Set to True to disable self-unenrollments via REST API. + # This also hides the "Unenroll" button on the Learner Dashboard. + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2021-10-11 + # .. toggle_warnings: For consistency in user experience, keep the value in sync with the setting of the same name + # in the LMS and CMS. + # .. toggle_tickets: 'https://github.com/open-craft/edx-platform/pull/429' + 'DISABLE_UNENROLLMENT': False, } # Specifies extra XBlock fields that should available when requested via the Course Blocks API diff --git a/lms/templates/dashboard.html b/lms/templates/dashboard.html index 0543f827a00..65345f4d305 100644 --- a/lms/templates/dashboard.html +++ b/lms/templates/dashboard.html @@ -209,7 +209,12 @@ cert_status = cert_statuses.get(session_id) can_refund_entitlement = entitlement and entitlement.is_entitlement_refundable() partner_managed_enrollment = enrollment.mode == 'masters' - can_unenroll = False if partner_managed_enrollment else (not cert_status) or cert_status.get('can_unenroll') if not unfulfilled_entitlement else False + # checks if we can unenroll based on the value of partner_managed_enrollment + can_unenroll_partner_managed_enrollment = False if partner_managed_enrollment else (not cert_status) + # checks if we can unenroll based on the value of unfulfilled_entitlement + can_unenroll_unfulfilled_entitlement = cert_status.get('can_unenroll') if cert_status and not unfulfilled_entitlement else False + # compares the three different parameters by which we can unenroll + can_unenroll = (can_unenroll_partner_managed_enrollment or can_unenroll_unfulfilled_entitlement) and not disable_unenrollment credit_status = credit_statuses.get(session_id) course_mode_info = all_course_modes.get(session_id) is_paid_course = True if entitlement else (session_id in enrolled_courses_either_paid) diff --git a/lms/templates/dashboard/_dashboard_course_listing.html b/lms/templates/dashboard/_dashboard_course_listing.html index 32bbffd0d24..35afee648da 100644 --- a/lms/templates/dashboard/_dashboard_course_listing.html +++ b/lms/templates/dashboard/_dashboard_course_listing.html @@ -250,11 +250,11 @@

% endif ## We should only show the gear dropdown if the user is able to refund/unenroll from their entitlement - ## and/or if they have selected a course run and email_settings are enabled + ## and/or if they have selected a course run, unenrollment is not disabled, and email_settings are enabled ## as these are the only actions currently available % if entitlement and (can_refund_entitlement or show_email_settings): <%include file='_dashboard_entitlement_actions.html' args='course_overview=course_overview,entitlement=entitlement,dashboard_index=dashboard_index, can_refund_entitlement=can_refund_entitlement, show_email_settings=show_email_settings'/> - % elif not entitlement: + % elif not entitlement and (can_unenroll or partner_managed_enrollment or show_email_settings):
').format(Text(token))) +class HomepageRenderStarted(OpenEdxPublicFilter): + """ + Custom class used to create homepage render filters and its custom methods. + """ + + filter_type = "org.openedx.learning.homepage.render.started.v1" + + class RedirectToPage(OpenEdxFilterException): + """ + Custom class used to stop the homepage rendering process. + """ + + def __init__(self, message, redirect_to=""): + """ + Override init that defines specific arguments used in the homepage render process. + + Arguments: + message: error message for the exception. + redirect_to: URL to redirect to. + """ + super().__init__(message, redirect_to=redirect_to) + + class RenderInvalidHomepage(OpenEdxFilterException): + """ + Custom class used to stop the homepage render process. + """ + + def __init__(self, message, index_template="", template_context=None): + """ + Override init that defines specific arguments used in the index render process. + + Arguments: + message: error message for the exception. + index_template: template path rendered instead. + template_context: context used to the new index_template. + """ + super().__init__( + message, + index_template=index_template, + template_context=template_context, + ) + + class RenderCustomResponse(OpenEdxFilterException): + """ + Custom class used to stop the homepage rendering process. + """ + + def __init__(self, message, response=None): + """ + Override init that defines specific arguments used in the homepage render process. + + Arguments: + message: error message for the exception. + response: custom response which will be returned by the homepage view. + """ + super().__init__( + message, + response=response, + ) + + @classmethod + def run_filter(cls, context, template_name): + """ + Execute a filter with the signature specified. + + Arguments: + context (dict): context dictionary for homepage template. + template_name (str): template name to be rendered by the homepage. + """ + data = super().run_pipeline(context=context, template_name=template_name) + return data.get("context"), data.get("template_name") + + # NOTE: This view is not linked to directly--it is called from # branding/views.py:index(), which is cached for anonymous users. # This means that it should always return the same thing for anon @@ -163,7 +239,23 @@ def index(request, extra_context=None, user=AnonymousUser()): # Add marketable programs to the context. context['programs_list'] = get_programs_with_type(request.site, include_hidden=False) - return render_to_response('index.html', context) + index_template = 'index.html' + try: + # .. filter_implemented_name: HomepageRenderStarted + # .. filter_type: org.openedx.learning.homepage.render.started.v1 + context, index_template = HomepageRenderStarted.run_filter( + context=context, template_name=index_template, + ) + except HomepageRenderStarted.RenderInvalidHomepage as exc: + response = render_to_response(exc.index_template, exc.template_context) # pylint: disable=no-member + except HomepageRenderStarted.RedirectToPage as exc: + response = HttpResponseRedirect(exc.redirect_to or reverse('account_settings')) + except HomepageRenderStarted.RenderCustomResponse as exc: + response = exc.response # pylint: disable=no-member + else: + response = render_to_response(index_template, context) + + return response def compose_activation_email(user, user_registration=None, route_enabled=False, profile_name='', redirect_url=None): diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 9098649b584..4700c328aba 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -17,7 +17,7 @@ from django.core.exceptions import PermissionDenied from django.db import transaction from django.db.models import Q, prefetch_related_objects -from django.http import Http404, HttpResponse, HttpResponseBadRequest, HttpResponseForbidden +from django.http import Http404, HttpResponse, HttpResponseBadRequest, HttpResponseForbidden, HttpResponseRedirect from django.shortcuts import redirect from django.template.context_processors import csrf from django.urls import reverse @@ -39,6 +39,8 @@ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_filters.learning.filters import CourseAboutRenderStarted +from openedx_filters.exceptions import OpenEdxFilterException +from openedx_filters.tooling import OpenEdxPublicFilter from pytz import UTC from requests.exceptions import ConnectionError, Timeout # pylint: disable=redefined-builtin from rest_framework import status @@ -266,6 +268,79 @@ def user_groups(user): return group_names +class CatalogRenderStarted(OpenEdxPublicFilter): + """ + Custom class used to create catalog render filters and its custom methods. + """ + + filter_type = "org.openedx.learning.catalog.render.started.v1" + + class RedirectToPage(OpenEdxFilterException): + """ + Custom class used to stop the catalog rendering process. + """ + + def __init__(self, message, redirect_to=""): + """ + Override init that defines specific arguments used in the catalog render process. + + Arguments: + message: error message for the exception. + redirect_to: URL to redirect to. + """ + super().__init__(message, redirect_to=redirect_to) + + class RenderInvalidCatalog(OpenEdxFilterException): + """ + Custom class used to stop the catalog render process. + """ + + def __init__(self, message, courses_template="", template_context=None): + """ + Override init that defines specific arguments used in the index render process. + + Arguments: + message: error message for the exception. + courses_template: template path rendered instead. + template_context: context used to the new courses_template. + """ + super().__init__( + message, + courses_template=courses_template, + template_context=template_context, + ) + + class RenderCustomResponse(OpenEdxFilterException): + """ + Custom class used to stop the catalog rendering process. + """ + + def __init__(self, message, response=None): + """ + Override init that defines specific arguments used in the catalog render process. + + Arguments: + message: error message for the exception. + response: custom response which will be returned by the catalog view. + """ + super().__init__( + message, + response=response, + ) + + @classmethod + def run_filter(cls, context, template_name): + """ + Execute a filter with the signature specified. + + Arguments: + context (dict): context dictionary for catalog template. + template_name (str): template name to be rendered by the catalog. + """ + data = super().run_pipeline(context=context, template_name=template_name) + return data.get("context"), data.get("template_name") + + @ensure_csrf_cookie @cache_if_anonymous() def courses(request): @@ -286,14 +361,28 @@ def courses(request): # Add marketable programs to the context. programs_list = get_programs_with_type(request.site, include_hidden=False) - return render_to_response( - "courseware/courses.html", - { - 'courses': courses_list, - 'course_discovery_meanings': course_discovery_meanings, - 'programs_list': programs_list, - } - ) + courses_template = "courseware/courses.html" + context = { + 'courses': courses_list, + 'course_discovery_meanings': course_discovery_meanings, + 'programs_list': programs_list, + } + try: + # .. filter_implemented_name: CatalogRenderStarted + # .. filter_type: org.openedx.learning.catalog.render.started.v1 + context, courses_template = CatalogRenderStarted.run_filter( + context=context, template_name=courses_template, + ) + except CatalogRenderStarted.RenderInvalidCatalog as exc: + response = render_to_response(exc.courses_template, exc.template_context) # pylint: disable=no-member + except CatalogRenderStarted.RedirectToPage as exc: + response = HttpResponseRedirect(exc.redirect_to or reverse('account_settings')) + except CatalogRenderStarted.RenderCustomResponse as exc: + response = exc.response # pylint: disable=no-member + else: + response = render_to_response(courses_template, context) + + return response class PerUserVideoMetadataThrottle(UserRateThrottle): From 826016ad89f5ea35fe466cd739af507c81615c9b Mon Sep 17 00:00:00 2001 From: Alejandro Cardenas Date: Tue, 4 Oct 2022 10:09:47 -0500 Subject: [PATCH 25/31] perf: add lru_cache to improve performance with multiple themes These changes should improve the performance caused by the file I/O when it's running in docker, using lru_cache to save thousands of calls to listdir when running with a handful of themes defined in COMPREHENSIVE_THEME_DIRS. --- .../courseware/tests/test_comprehensive_theming.py | 6 ++++++ openedx/core/djangoapps/theming/helpers.py | 2 ++ openedx/core/djangoapps/theming/helpers_dirs.py | 2 ++ openedx/core/djangoapps/theming/tests/test_helpers.py | 10 ++++++++++ 4 files changed, 20 insertions(+) diff --git a/lms/djangoapps/courseware/tests/test_comprehensive_theming.py b/lms/djangoapps/courseware/tests/test_comprehensive_theming.py index 43eb9663a44..0e3bcf6b16f 100644 --- a/lms/djangoapps/courseware/tests/test_comprehensive_theming.py +++ b/lms/djangoapps/courseware/tests/test_comprehensive_theming.py @@ -8,6 +8,8 @@ from common.djangoapps import edxmako from openedx.core.djangoapps.theming.tests.test_util import with_comprehensive_theme +from openedx.core.djangoapps.theming.helpers import get_themes +from openedx.core.djangoapps.theming.helpers_dirs import get_theme_dirs from openedx.core.lib.tempdir import create_symlink, delete_symlink, mkdtemp_clean @@ -20,6 +22,10 @@ def setUp(self): # Clear the internal staticfiles caches, to get test isolation. staticfiles.finders.get_finder.cache_clear() + # Clear cache on get_theme methods. + get_themes.cache_clear() + get_theme_dirs.cache_clear() + @with_comprehensive_theme('red-theme') def test_red_footer(self): """ diff --git a/openedx/core/djangoapps/theming/helpers.py b/openedx/core/djangoapps/theming/helpers.py index 7b6745846ef..6b35680bc03 100644 --- a/openedx/core/djangoapps/theming/helpers.py +++ b/openedx/core/djangoapps/theming/helpers.py @@ -23,6 +23,7 @@ get_themes_unchecked ) from openedx.core.lib.cache_utils import request_cached +from functools import lru_cache logger = getLogger(__name__) # pylint: disable=invalid-name @@ -256,6 +257,7 @@ def theme_exists(theme_name, themes_dir=None): return False +@lru_cache def get_themes(themes_dir=None): """ get a list of all themes known to the system. diff --git a/openedx/core/djangoapps/theming/helpers_dirs.py b/openedx/core/djangoapps/theming/helpers_dirs.py index f179998fbd5..9e40749e50c 100644 --- a/openedx/core/djangoapps/theming/helpers_dirs.py +++ b/openedx/core/djangoapps/theming/helpers_dirs.py @@ -7,6 +7,7 @@ import os from path import Path +from functools import lru_cache def get_theme_base_dirs_from_settings(theme_base_dirs=None): @@ -49,6 +50,7 @@ def get_themes_unchecked(themes_base_dirs, project_root=None): return themes +@lru_cache def get_theme_dirs(themes_base_dir=None): """ Get all the theme dirs directly under a given base dir. diff --git a/openedx/core/djangoapps/theming/tests/test_helpers.py b/openedx/core/djangoapps/theming/tests/test_helpers.py index 57d523d1b82..2e8e444017e 100644 --- a/openedx/core/djangoapps/theming/tests/test_helpers.py +++ b/openedx/core/djangoapps/theming/tests/test_helpers.py @@ -16,6 +16,7 @@ get_themes, strip_site_theme_templates_path ) +from openedx.core.djangoapps.theming.helpers_dirs import get_theme_dirs from openedx.core.djangoapps.theming.tests.test_util import with_comprehensive_theme from openedx.core.djangolib.testing.utils import skip_unless_cms, skip_unless_lms @@ -23,6 +24,15 @@ class TestHelpers(TestCase): """Test comprehensive theming helper functions.""" + def setUp(self): + """ + Clear cache on get_theme methods. + """ + super().setUp() + + get_themes.cache_clear() + get_theme_dirs.cache_clear() + def test_get_themes(self): """ Tests template paths are returned from enabled theme. From 4c08d69c4af99ecca604b144b4e2bea7d8089174 Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Sat, 26 Nov 2022 10:58:55 -0500 Subject: [PATCH 26/31] fix: update xblock-drag-and-drop for a high level security alert --- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/github.in | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 4ed2a7d886c..a9e66569357 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -1056,7 +1056,7 @@ xblock==1.5.1 # xblock-google-drive # xblock-poll # xblock-utils -xblock-drag-and-drop-v2 @ git+https://github.com/edx-solutions/xblock-drag-and-drop-v2@v2.3.5 +xblock-drag-and-drop-v2 @ git+https://github.com/edx-solutions/xblock-drag-and-drop-v2@v3.0.0 # via -r requirements/edx/github.in xblock-poll @ git+https://github.com/open-craft/xblock-poll@922cd36fb1c3cfe00b4ce03b19a13185d136447d # via -r requirements/edx/github.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index d627f5e9477..b3a90f42491 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -1537,7 +1537,7 @@ xblock==1.5.1 # xblock-google-drive # xblock-poll # xblock-utils -xblock-drag-and-drop-v2 @ git+https://github.com/edx-solutions/xblock-drag-and-drop-v2@v2.3.5 +xblock-drag-and-drop-v2 @ git+https://github.com/edx-solutions/xblock-drag-and-drop-v2@v3.0.0 # via -r requirements/edx/testing.txt xblock-poll @ git+https://github.com/open-craft/xblock-poll@922cd36fb1c3cfe00b4ce03b19a13185d136447d # via -r requirements/edx/testing.txt diff --git a/requirements/edx/github.in b/requirements/edx/github.in index c2f4f0333c7..301bbe7689c 100644 --- a/requirements/edx/github.in +++ b/requirements/edx/github.in @@ -74,4 +74,4 @@ git+https://github.com/edx/django-require.git@0c54adb167142383b26ea6b3edecc32118 # Third Party XBlocks git+https://github.com/open-craft/xblock-poll@922cd36fb1c3cfe00b4ce03b19a13185d136447d#egg=xblock-poll==1.10.2 -git+https://github.com/edx-solutions/xblock-drag-and-drop-v2@v2.3.5#egg=xblock-drag-and-drop-v2==2.3.5 +git+https://github.com/edx-solutions/xblock-drag-and-drop-v2@v3.0.0#egg=xblock-drag-and-drop-v2==3.0.0 From 91fe2877a331d0d36b48f0395a91bb7afd7e8f6d Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Wed, 30 Nov 2022 16:28:50 -0500 Subject: [PATCH 27/31] chore: compiled dependencies --- requirements/common_constraints.txt | 9 ++++++++- requirements/edx-sandbox/py35.txt | 1 + requirements/edx/base.txt | 7 +++---- requirements/edx/development.txt | 4 ++-- requirements/edx/testing.txt | 7 +++---- 5 files changed, 17 insertions(+), 11 deletions(-) diff --git a/requirements/common_constraints.txt b/requirements/common_constraints.txt index 332eaa4e040..97e0e4e2038 100644 --- a/requirements/common_constraints.txt +++ b/requirements/common_constraints.txt @@ -18,8 +18,15 @@ # using LTS django version - +Django<4.0 # elasticsearch>=7.14.0 includes breaking changes in it which caused issues in discovery upgrade process. # elastic search changelog: https://www.elastic.co/guide/en/enterprise-search/master/release-notes-7.14.0.html elasticsearch<7.14.0 + +# setuptools==60.0 had breaking changes and busted several service's pipeline. +# Details can be found here: https://github.com/pypa/setuptools/issues/2940 +setuptools<60 + +# django-simple-history>3.0.0 adds indexing and causes a lot of migrations to be affected +django-simple-history==3.0.0 diff --git a/requirements/edx-sandbox/py35.txt b/requirements/edx-sandbox/py35.txt index fe3e3da1bea..a7f9e007a9c 100644 --- a/requirements/edx-sandbox/py35.txt +++ b/requirements/edx-sandbox/py35.txt @@ -53,6 +53,7 @@ networkx==2.2 # -r requirements/edx-sandbox/py35.in nltk==3.6.2 # via + # -c requirements/edx-sandbox/py35-constraints.txt # -r requirements/edx-sandbox/py35.in # chem numpy==1.16.5 diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index a9e66569357..827522042fb 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -149,9 +149,7 @@ cryptography==35.0.0 cssutils==2.3.0 # via pynliner ddt==1.4.4 - # via - # xblock-drag-and-drop-v2 - # xblock-poll + # via xblock-poll defusedxml==0.7.1 # via # -r requirements/edx/base.in @@ -165,6 +163,7 @@ deprecated==1.2.13 # via jwcrypto django==3.2.13 # via + # -c requirements/edx/../common_constraints.txt # -c requirements/edx/../constraints.txt # -r requirements/edx/base.in # django-appconf @@ -327,6 +326,7 @@ django-ses==2.3.0 # via -r requirements/edx/base.in django-simple-history==3.0.0 # via + # -c requirements/edx/../common_constraints.txt # -r requirements/edx/base.in # edx-enterprise # edx-organizations @@ -641,7 +641,6 @@ maxminddb==2.2.0 mock==4.0.3 # via # -r requirements/edx/paver.txt - # xblock-drag-and-drop-v2 # xblock-poll mongodbproxy @ git+https://github.com/edx/MongoDBProxy.git@d92bafe9888d2940f647a7b2b2383b29c752f35a # via -r requirements/edx/github.in diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index b3a90f42491..e30ab1a188d 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -220,7 +220,6 @@ cssutils==2.3.0 ddt==1.4.4 # via # -r requirements/edx/testing.txt - # xblock-drag-and-drop-v2 # xblock-poll defusedxml==0.7.1 # via @@ -245,6 +244,7 @@ distlib==0.3.3 # virtualenv django==3.2.13 # via + # -c requirements/edx/../common_constraints.txt # -c requirements/edx/../constraints.txt # -r requirements/edx/testing.txt # django-appconf @@ -420,6 +420,7 @@ django-ses==2.3.0 # via -r requirements/edx/testing.txt django-simple-history==3.0.0 # via + # -c requirements/edx/../common_constraints.txt # -r requirements/edx/testing.txt # edx-enterprise # edx-organizations @@ -865,7 +866,6 @@ mistune==0.8.4 mock==4.0.3 # via # -r requirements/edx/testing.txt - # xblock-drag-and-drop-v2 # xblock-poll mongodbproxy @ git+https://github.com/edx/MongoDBProxy.git@d92bafe9888d2940f647a7b2b2383b29c752f35a # via -r requirements/edx/testing.txt diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 1d1f8bb7d50..03a5961da32 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -73,7 +73,6 @@ attrs==21.2.0 # aiohttp # edx-ace # openedx-events - # outcome # pytest babel==2.9.1 # via @@ -210,7 +209,6 @@ ddt==1.4.4 # via # -r requirements/edx/base.txt # -r requirements/edx/testing.in - # xblock-drag-and-drop-v2 # xblock-poll defusedxml==0.7.1 # via @@ -232,6 +230,7 @@ diff-cover==4.0.0 distlib==0.3.3 # via virtualenv # via + # -c requirements/edx/../common_constraints.txt # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt # django-appconf @@ -404,6 +403,7 @@ django-ses==2.3.0 # via -r requirements/edx/base.txt django-simple-history==3.0.0 # via + # -c requirements/edx/../common_constraints.txt # -r requirements/edx/base.txt # edx-enterprise # edx-organizations @@ -818,7 +818,6 @@ mccabe==0.6.1 mock==4.0.3 # via # -r requirements/edx/base.txt - # xblock-drag-and-drop-v2 # xblock-poll mongodbproxy @ git+https://github.com/edx/MongoDBProxy.git@d92bafe9888d2940f647a7b2b2383b29c752f35a # via -r requirements/edx/base.txt @@ -1413,7 +1412,7 @@ xblock==1.5.1 # xblock-google-drive # xblock-poll # xblock-utils -xblock-drag-and-drop-v2 @ git+https://github.com/edx-solutions/xblock-drag-and-drop-v2@v2.3.5 +xblock-drag-and-drop-v2 @ git+https://github.com/edx-solutions/xblock-drag-and-drop-v2@v3.0.0 # via -r requirements/edx/base.txt xblock-poll @ git+https://github.com/open-craft/xblock-poll@922cd36fb1c3cfe00b4ce03b19a13185d136447d # via -r requirements/edx/base.txt From 5c65a714e7aac0639cedac5ab6aa0aa4188af743 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mar=C3=ADa=20Fernanda=20Magallanes?= <35668326+MaferMazu@users.noreply.github.com> Date: Mon, 17 Jul 2023 19:29:51 -0500 Subject: [PATCH 28/31] feat: Add GA 4 support to edX platform (#762) Merge pull request #32032 from raccoongang/sagirov/tCRIL_GA-18 [FC-0014] Add GA 4 support to edX platform Co-authored-by: Brian Mesick <112640379+bmtcril@users.noreply.github.com> --- lms/envs/common.py | 1 + lms/envs/production.py | 1 + lms/templates/certificates/accomplishment-base.html | 13 +++++++++++++ lms/templates/main.html | 12 ++++++++++++ lms/templates/main_django.html | 12 ++++++++++++ .../templatetags/configuration.py | 9 +++++++++ 6 files changed, 48 insertions(+) diff --git a/lms/envs/common.py b/lms/envs/common.py index 5bfb4ba5e19..40facc565b7 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1442,6 +1442,7 @@ def _make_mako_template_dirs(settings): GOOGLE_SITE_VERIFICATION_ID = '' GOOGLE_ANALYTICS_LINKEDIN = 'GOOGLE_ANALYTICS_LINKEDIN_DUMMY' GOOGLE_ANALYTICS_TRACKING_ID = None +GOOGLE_ANALYTICS_4_ID = None ######################## BRANCH.IO ########################### BRANCH_IO_KEY = '' diff --git a/lms/envs/production.py b/lms/envs/production.py index e68f613ba66..6c7e0815a4a 100644 --- a/lms/envs/production.py +++ b/lms/envs/production.py @@ -687,6 +687,7 @@ def get_env_setting(setting): GOOGLE_ANALYTICS_TRACKING_ID = AUTH_TOKENS.get('GOOGLE_ANALYTICS_TRACKING_ID') GOOGLE_ANALYTICS_LINKEDIN = AUTH_TOKENS.get('GOOGLE_ANALYTICS_LINKEDIN') GOOGLE_SITE_VERIFICATION_ID = ENV_TOKENS.get('GOOGLE_SITE_VERIFICATION_ID') +GOOGLE_ANALYTICS_4_ID = AUTH_TOKENS.get('GOOGLE_ANALYTICS_4_ID') ##### BRANCH.IO KEY ##### BRANCH_IO_KEY = AUTH_TOKENS.get('BRANCH_IO_KEY') diff --git a/lms/templates/certificates/accomplishment-base.html b/lms/templates/certificates/accomplishment-base.html index 1fe0456469c..554bb607ccb 100644 --- a/lms/templates/certificates/accomplishment-base.html +++ b/lms/templates/certificates/accomplishment-base.html @@ -19,6 +19,19 @@ ${document_title} <%static:css group='style-certificates'/> + + + <% ga_4_id = static.get_value("GOOGLE_ANALYTICS_4_ID", settings.GOOGLE_ANALYTICS_4_ID) %> + % if ga_4_id: + + + % endif diff --git a/lms/templates/main.html b/lms/templates/main.html index 83416317ba8..a6bac477514 100644 --- a/lms/templates/main.html +++ b/lms/templates/main.html @@ -163,6 +163,18 @@ % endif +<% ga_4_id = static.get_value("GOOGLE_ANALYTICS_4_ID", settings.GOOGLE_ANALYTICS_4_ID) %> +% if ga_4_id: + + +% endif + <% branch_key = static.get_value("BRANCH_IO_KEY", settings.BRANCH_IO_KEY) %> % if branch_key and not is_from_mobile_app: + + {% endif %} + diff --git a/openedx/core/djangoapps/site_configuration/templatetags/configuration.py b/openedx/core/djangoapps/site_configuration/templatetags/configuration.py index b5242a53b5b..9de10d3bb07 100644 --- a/openedx/core/djangoapps/site_configuration/templatetags/configuration.py +++ b/openedx/core/djangoapps/site_configuration/templatetags/configuration.py @@ -65,3 +65,12 @@ def microsite_template_path(template_name): """ template_name = theming_helpers.get_template_path(template_name) return template_name[1:] if template_name[0] == '/' else template_name + + +@register.simple_tag +def google_analytics_4_id(): + """ + Django template tag that outputs the GOOGLE_ANALYTICS_4_ID: + {% google_analytics_4_id %} + """ + return configuration_helpers.get_value("GOOGLE_ANALYTICS_4_ID", settings.GOOGLE_ANALYTICS_4_ID) From 6711d7b72b71f523fa72aa0509aa1081af73f577 Mon Sep 17 00:00:00 2001 From: henrrypg Date: Mon, 7 Nov 2022 15:34:34 -0500 Subject: [PATCH 29/31] feat: add custom field on register form --- .../user_api/accounts/settings_views.py | 104 ++++++++++ .../user_authn/views/registration_form.py | 178 ++++++++++++++++++ 2 files changed, 282 insertions(+) diff --git a/openedx/core/djangoapps/user_api/accounts/settings_views.py b/openedx/core/djangoapps/user_api/accounts/settings_views.py index ab321638d2f..7c7654cfcde 100644 --- a/openedx/core/djangoapps/user_api/accounts/settings_views.py +++ b/openedx/core/djangoapps/user_api/accounts/settings_views.py @@ -8,12 +8,16 @@ from django.conf import settings from django.contrib import messages from django.contrib.auth.decorators import login_required +from django.core.exceptions import ImproperlyConfigured from django.shortcuts import redirect from django.urls import reverse from django.utils.translation import gettext as _ from django.views.decorators.http import require_http_methods from django_countries import countries +from openedx_filters import PipelineStep +from openedx_filters.tooling import OpenEdxPublicFilter +from openedx_filters.exceptions import OpenEdxFilterException from common.djangoapps import third_party_auth from common.djangoapps.edxmako.shortcuts import render_to_response from lms.djangoapps.commerce.models import CommerceConfiguration @@ -39,6 +43,100 @@ log = logging.getLogger(__name__) +class AddCustomOptionsOnAccountSettings(PipelineStep): + """ Pipeline used to add custom option fields in account settings. + + Example usage: + + Add the following configurations to your configuration file: + "OPEN_EDX_FILTERS_CONFIG": { + "org.openedx.learning.student.settings.render.started.v1": { + "fail_silently": false, + "pipeline": [ + "eox_tenant.samples.pipeline.AddCustomOptionsOnAccountSettings" + ] + } + } + """ + + def run_filter(self, context): # pylint: disable=arguments-differ + """ Run the pipeline filter. """ + extended_profile_fields = context.get("extended_profile_fields", []) + + custom_options, field_labels_map = self._get_custom_context(extended_profile_fields) # pylint: disable=line-too-long + + extended_profile_field_options = configuration_helpers.get_value('EXTRA_FIELD_OPTIONS', custom_options) # pylint: disable=line-too-long + extended_profile_field_option_tuples = {} + for field in extended_profile_field_options.keys(): + field_options = extended_profile_field_options[field] + extended_profile_field_option_tuples[field] = [(option.lower(), option) for option in field_options] # pylint: disable=line-too-long + + for field in custom_options: + field_dict = { + "field_name": field, + "field_label": field_labels_map.get(field, field), + } + + field_options = extended_profile_field_option_tuples.get(field) + if field_options: + field_dict["field_type"] = "ListField" + field_dict["field_options"] = field_options + else: + field_dict["field_type"] = "TextField" + + field_index = next((index for (index, d) in enumerate(extended_profile_fields) if d["field_name"] == field_dict["field_name"]), None) # pylint: disable=line-too-long + if field_index is not None: + context["extended_profile_fields"][field_index] = field_dict + return context + + def _get_custom_context(self, extended_profile_fields): + """ Get custom context for the field. """ + field_labels = {} + field_options = {} + custom_fields = getattr(settings, "EDNX_CUSTOM_REGISTRATION_FIELDS", []) + + for field in custom_fields: + field_name = field.get("name") + + if not field_name: # Required to identify the field. + msg = "Custom fields must have a `name` defined in their configuration." + raise ImproperlyConfigured(msg) + + field_label = field.get("label") + if not any(extended_field['field_name'] == field_name for extended_field in extended_profile_fields) and field_label: # pylint: disable=line-too-long + field_labels[field_name] = _(field_label) # pylint: disable=translation-of-non-string + + options = field.get("options") + + if options: + field_options[field_name] = options + + return field_options, field_labels + + +class AccountSettingsRenderStarted(OpenEdxPublicFilter): + """ Custom class used to create Account settings filters. """ + + filter_type = "org.openedx.learning.student.settings.render.started.v1" + + class ErrorFilteringContext(OpenEdxFilterException): + """ + Custom class used catch exceptions when filtering the context. + """ + + @classmethod + def run_filter(cls, context): + """ + Execute a filter with the signature specified. + + Arguments: + context (dict): context for the account settings page. + """ + data = super().run_pipeline(context=context) + context = data.get("context") + return context + + @login_required @require_http_methods(['GET']) def account_settings(request): @@ -72,6 +170,12 @@ def account_settings(request): return redirect(url) context = account_settings_context(request) + + try: + context = AccountSettingsRenderStarted().run_filter(context=context) + except AccountSettingsRenderStarted.ErrorFilteringContext as exc: + raise ImproperlyConfigured(f'Pipeline configuration error: {exc}') from exc + return render_to_response('student_account/account_settings.html', context) diff --git a/openedx/core/djangoapps/user_authn/views/registration_form.py b/openedx/core/djangoapps/user_authn/views/registration_form.py index d530f493a9d..9fb2749a30b 100644 --- a/openedx/core/djangoapps/user_authn/views/registration_form.py +++ b/openedx/core/djangoapps/user_authn/views/registration_form.py @@ -3,6 +3,7 @@ """ import copy +from collections import OrderedDict from importlib import import_module import re @@ -16,6 +17,9 @@ from django.utils.translation import gettext as _ from django_countries import countries +from openedx_filters import PipelineStep +from openedx_filters.tooling import OpenEdxPublicFilter +from openedx_filters.exceptions import OpenEdxFilterException from common.djangoapps import third_party_auth from common.djangoapps.edxmako.shortcuts import marketing_link from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers @@ -36,6 +40,174 @@ ) +class AddCustomFieldsBeforeRegistration(PipelineStep): + """ Pipeline used to add custom fields to the registration form. + + Example usage: + + Add the following configurations to your configuration file: + "OPEN_EDX_FILTERS_CONFIG": { + "org.openedx.learning.student.registration.render.started.v1": { + "fail_silently": false, + "pipeline": [ + "eox_tenant.samples.pipeline.AddCustomFieldsBeforeRegistration" + ] + } + } + """ + + def run_filter(self, form_desc): # pylint: disable=arguments-differ + """Run the pipeline filter.""" + + extra_fields = self._get_extra_fields() + + for field_name in extra_fields: + extra_field = {"field_name": field_name} + self._add_custom_field( + form_desc, + required=self._is_field_required(field_name), + **extra_field + ) + fields = form_desc.fields + fields = [field['name'] for field in fields] + + field_order = configuration_helpers.get_value('REGISTRATION_FIELD_ORDER') + if not field_order: + field_order = settings.REGISTRATION_FIELD_ORDER or fields + + if set(fields) != set(field_order): + difference = set(fields).difference(set(field_order)) + field_order.extend(sorted(difference)) + + ordered_fields = [] + for field in field_order: + for form_field in form_desc.fields: + if field == form_field['name']: + ordered_fields.append(form_field) + break + + form_desc.fields = ordered_fields + return form_desc + + def _get_extra_fields(self): + """Returns the list of extra fields to include in the registration form. + Returns: + list of strings + """ + extended_profile_fields = [field.lower() for field in getattr(settings, 'extended_profile_fields', [])] # lint-amnesty, pylint: disable=line-too-long + + return list(OrderedDict.fromkeys(extended_profile_fields)) + + def _is_field_required(self, field_name): + """Check whether a field is required based on Django settings. """ + _extra_fields_setting = copy.deepcopy( + configuration_helpers.get_value('REGISTRATION_EXTRA_FIELDS') + ) + if not _extra_fields_setting: + _extra_fields_setting = copy.deepcopy(settings.REGISTRATION_EXTRA_FIELDS) + + return _extra_fields_setting.get(field_name) == "required" + + def _get_custom_field_dict(self, field_name): + """Given a field name searches for its definition dictionary. + Arguments: + field_name (str): the name of the field to search for. + """ + custom_fields = getattr(settings, "EDNX_CUSTOM_REGISTRATION_FIELDS", []) + for field in custom_fields: + if field.get("name").lower() == field_name: + return field + return {} + + def _get_custom_html_override(self, text_field, html_piece=None): + """Overrides field with html piece. + Arguments: + text_field: field to override. It must have the following format: + "Here {} goes the HTML piece." In `{}` will be inserted the HTML piece. + Keyword Arguments: + html_piece: string containing HTML components to be inserted. + """ + if html_piece: + html_piece = HTML(html_piece) if isinstance(html_piece, str) else "" + return Text(_(text_field)).format(html_piece) # pylint: disable=translation-of-non-string + return text_field + + def _add_custom_field(self, form_desc, required=True, **kwargs): + """Adds custom fields to a form description. + Arguments: + form_desc: A form description + Keyword Arguments: + required (bool): Whether this field is required; defaults to False + field_name (str): Name used to get field information when creating it. + """ + field_name = kwargs.pop("field_name") + if field_name in getattr(settings, "EDNX_IGNORE_REGISTER_FIELDS", []): + return + + custom_field_dict = self._get_custom_field_dict(field_name) + if not custom_field_dict: + return + + # Check to convert options: + field_options = custom_field_dict.get("options") + if isinstance(field_options, dict): + field_options = [(str(value.lower()), name) for value, name in field_options.items()] + elif isinstance(field_options, list): + field_options = [(str(value.lower()), value) for value in field_options] + + # Set default option if applies: + default_option = custom_field_dict.get("default") + if default_option: + form_desc.override_field_properties( + field_name, + default=default_option + ) + + field_type = custom_field_dict.get("type") + + form_desc.add_field( + field_name, + label=self._get_custom_html_override( + custom_field_dict.get("label"), + custom_field_dict.get("html_override"), + ), + field_type=field_type, + options=field_options, + instructions=custom_field_dict.get("instructions"), + placeholder=custom_field_dict.get("placeholder"), + restrictions=custom_field_dict.get("restrictions"), + include_default_option=bool(default_option) or field_type == "select", + required=required, + error_messages=custom_field_dict.get("errorMessages") + ) + + +class StudentRegistrationRenderStarted(OpenEdxPublicFilter): + """ + Custom class used to add custom fields to the registration form. + """ + + filter_type = "org.openedx.learning.student.registration.render.started.v1" + + class ErrorFilteringFormDescription(OpenEdxFilterException): + """ + Custom class used catch exceptions when filtering form description. + """ + + @classmethod + def run_filter(cls, form_desc): + """ + Execute a filter with the signature specified. + + Arguments: + form_desc (dict): description form specifying the fields to be rendered + in the registration form. + """ + data = super().run_pipeline(form_desc=form_desc) + form_data = data.get("form_desc") + return form_data + + class TrueCheckbox(widgets.CheckboxInput): """ A checkbox widget that only accepts "true" (case-insensitive) as true. @@ -455,6 +627,12 @@ def get_registration_form(self, request): if field['name'] == 'confirm_email': del form_desc.fields[index] break + + try: + form_desc = StudentRegistrationRenderStarted().run_filter(form_desc) + except StudentRegistrationRenderStarted.ErrorFilteringFormDescription as exc: + raise ImproperlyConfigured(f'Pipeline configuration error: {exc}') from exc + return form_desc def _get_registration_submit_url(self, request): From 35053e7a14f756a725a24d4c334e2d321d018b6b Mon Sep 17 00:00:00 2001 From: mariagrimaldi Date: Thu, 14 Jan 2021 13:19:34 -0400 Subject: [PATCH 30/31] fix: fix course requirements issue with proxy class (cherry picked from commit 4f62355304b8bed8d36f179a901f7bddd36a2a72) --- lms/djangoapps/certificates/models.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/certificates/models.py b/lms/djangoapps/certificates/models.py index 983446275a6..ceb9b5804f1 100644 --- a/lms/djangoapps/certificates/models.py +++ b/lms/djangoapps/certificates/models.py @@ -465,8 +465,12 @@ def save(self, *args, **kwargs): # pylint: disable=signature-differs Credentials IDA. """ super().save(*args, **kwargs) + if self._meta.proxy: + sending_class = self._meta.proxy_for_model + else: + sending_class = self.__class__ COURSE_CERT_CHANGED.send_robust( - sender=self.__class__, + sender=sending_class, user=self.user, course_key=self.course_id, mode=self.mode, @@ -498,7 +502,7 @@ def save(self, *args, **kwargs): # pylint: disable=signature-differs if CertificateStatuses.is_passing_status(self.status): COURSE_CERT_AWARDED.send_robust( - sender=self.__class__, + sender=sending_class, user=self.user, course_key=self.course_id, mode=self.mode, From a928a8d480e20a2902c6e7f096326fa07f3c67f0 Mon Sep 17 00:00:00 2001 From: Deimer Morales <105317492+DeimerM@users.noreply.github.com> Date: Mon, 22 May 2023 20:58:19 -0500 Subject: [PATCH 31/31] fix: adding TypeError to compare scores function --- lms/djangoapps/grades/signals/handlers.py | 2 +- openedx/core/lib/grade_utils.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/grades/signals/handlers.py b/lms/djangoapps/grades/signals/handlers.py index 14089d74ea5..a3f605bbd16 100644 --- a/lms/djangoapps/grades/signals/handlers.py +++ b/lms/djangoapps/grades/signals/handlers.py @@ -152,7 +152,7 @@ def score_published_handler(sender, block, user, raw_earned, raw_possible, only_ if previous_score is not None: prev_raw_earned, prev_raw_possible = (previous_score.grade, previous_score.max_grade) - if not is_score_higher_or_equal(prev_raw_earned, prev_raw_possible, raw_earned, raw_possible): + if not is_score_higher_or_equal(prev_raw_earned, prev_raw_possible, raw_earned, raw_possible, True): update_score = False log.warning( "Grades: Rescore is not higher than previous: " diff --git a/openedx/core/lib/grade_utils.py b/openedx/core/lib/grade_utils.py index 2e7073e962d..48c262e85a5 100644 --- a/openedx/core/lib/grade_utils.py +++ b/openedx/core/lib/grade_utils.py @@ -17,14 +17,14 @@ def compare_scores(earned1, possible1, earned2, possible2, treat_undefined_as_ze """ try: percentage1 = float(earned1) / float(possible1) - except ZeroDivisionError: + except (ZeroDivisionError, TypeError): if not treat_undefined_as_zero: raise percentage1 = 0.0 try: percentage2 = float(earned2) / float(possible2) - except ZeroDivisionError: + except (ZeroDivisionError, TypeError): if not treat_undefined_as_zero: raise percentage2 = 0.0