From 229c5249a1aab3a60842d24fd41fe413b820b270 Mon Sep 17 00:00:00 2001 From: Ali Nawaz Date: Tue, 3 Dec 2024 14:14:34 +0500 Subject: [PATCH 1/7] docs: add ADR for jwt role throttling --- docs/decisions/0029-jwt-based-throttling.rst | 37 ++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 docs/decisions/0029-jwt-based-throttling.rst diff --git a/docs/decisions/0029-jwt-based-throttling.rst b/docs/decisions/0029-jwt-based-throttling.rst new file mode 100644 index 0000000000..39c4cc79a2 --- /dev/null +++ b/docs/decisions/0029-jwt-based-throttling.rst @@ -0,0 +1,37 @@ +29. JWT roles based Throttling +============================== + +Status +-------- +Accepted (Dec 2024) + +Context +--------- +Course Discovery APIs are used by a number of consumers like external partners and marketers, the LMS, Publisher, and the +enterprise-catalog service. One of these consumers that stands out is the Enterprise Learner Portal. Just like Publisher, this +portal is an MFE that queries discovery at runtime. However, unlike Publisher, where our users are mostly staff and partners, the +learner portal is intended to be used by regular enterprise learners and admins i.e those without any staff or privileged access. + +As of late, our regular throttling limits have proven to be a little too aggressive for some of these learners. As this is clearly +non-malicious traffic, we would like to find a way to set more lenient throttling limits for these learners. + +Decision +---------- +Since these learners authenticate with Discovery by using JWT tokens issued by the LMS, we have decided to use the `roles` key +in the JWT to identify them. Enterprise customers are guaranteed to have one of a small number of fixed roles assigned to them. +Once we identify that an incoming request's user has one of these roles in their JWT, we will enable higher rate limits for them. + +Consequences +-------------- +- Some regular users (i.e without staff or any django group privileges) will have privileged rate limits +- We will need to be vigilant to ensure that the list of roles we consider privileged is accurate i.e its entries should only + be associated to enterprise learners (i.e the users we need higher limits for). Furthermore, it should be impossible for + "other" users to attain those roles. + +Alternatives Considered +------------------------- +- Raise the throttle limits for everyone. However, that increases the possibility of scraping and service degradation attacks + by malicious actors +- Extend the user model in discovery to store identification information for users with enhanced throttle limits. However, that + conflicts with the responsibility of the LMS as the sole authentication provider. Furthermore, adding this information in + discovery for each enterprise user would needlessly complicate the enterprise onboarding and offboarding process. From b0d1058d389e07c718d7ccfdf39caea42d2291fa Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Wed, 24 Jul 2024 12:48:03 -0400 Subject: [PATCH 2/7] fix: increase the default throttle rate for enterprise users --- course_discovery/apps/api/tests/jwt_utils.py | 11 +++-- .../apps/core/tests/test_throttles.py | 43 ++++++++++++++++--- course_discovery/apps/core/throttles.py | 21 ++++++++- 3 files changed, 63 insertions(+), 12 deletions(-) diff --git a/course_discovery/apps/api/tests/jwt_utils.py b/course_discovery/apps/api/tests/jwt_utils.py index 4382c5288c..60f26bb189 100644 --- a/course_discovery/apps/api/tests/jwt_utils.py +++ b/course_discovery/apps/api/tests/jwt_utils.py @@ -5,11 +5,11 @@ from django.conf import settings -def generate_jwt_payload(user): +def generate_jwt_payload(user, payload=None): """Generate a valid JWT payload given a user.""" now = int(time()) ttl = 5 - return { + jwt_payload = { 'iss': settings.JWT_AUTH['JWT_ISSUER'], 'aud': settings.JWT_AUTH['JWT_AUDIENCE'], 'username': user.username, @@ -17,6 +17,9 @@ def generate_jwt_payload(user): 'iat': now, 'exp': now + ttl } + if payload: + jwt_payload.update(payload) + return jwt_payload def generate_jwt_token(payload): @@ -29,8 +32,8 @@ def generate_jwt_header(token): return f'JWT {token}' -def generate_jwt_header_for_user(user): - payload = generate_jwt_payload(user) +def generate_jwt_header_for_user(user, payload=None): + payload = generate_jwt_payload(user, payload) token = generate_jwt_token(payload) return generate_jwt_header(token) diff --git a/course_discovery/apps/core/tests/test_throttles.py b/course_discovery/apps/core/tests/test_throttles.py index f76c703399..7c3b662c9d 100644 --- a/course_discovery/apps/core/tests/test_throttles.py +++ b/course_discovery/apps/core/tests/test_throttles.py @@ -1,8 +1,10 @@ from unittest.mock import patch +import ddt from django.urls import reverse from rest_framework.test import APITestCase +from course_discovery.apps.api.tests.jwt_utils import generate_jwt_header_for_user from course_discovery.apps.api.tests.mixins import SiteMixin from course_discovery.apps.core.models import UserThrottleRate from course_discovery.apps.core.tests.factories import USER_PASSWORD, UserFactory @@ -10,6 +12,7 @@ from course_discovery.apps.publisher.tests.factories import GroupFactory +@ddt.ddt class RateLimitingExceededTest(SiteMixin, APITestCase): """ Testing rate limiting of API calls. @@ -26,7 +29,15 @@ def tearDown(self): super().tearDown() throttling_cache().clear() - def _make_requests(self, count=None): + def _build_jwt_headers(self, user, payload=None): + """ + Helper function for creating headers for the JWT authentication. + """ + token = generate_jwt_header_for_user(user, payload) + headers = {'HTTP_AUTHORIZATION': token} + return headers + + def _make_requests(self, count=None, **headers): """ Make multiple requests until the throttle's limit is exceeded. Returns @@ -37,19 +48,19 @@ def _make_requests(self, count=None): with patch('rest_framework.views.APIView.throttle_classes', (OverridableUserRateThrottle,)): with patch.object(OverridableUserRateThrottle, 'THROTTLE_RATES', user_rates): for __ in range(count - 1): - response = self.client.get(self.url) + response = self.client.get(self.url, **headers) assert response.status_code == 200 - response = self.client.get(self.url) + response = self.client.get(self.url, **headers) return response - def assert_rate_limit_successfully_exceeded(self): + def assert_rate_limit_successfully_exceeded(self, **headers): """ Asserts that the throttle's rate limit can be exceeded without encountering an error. """ - response = self._make_requests() + response = self._make_requests(**headers) assert response.status_code == 200 - def assert_rate_limited(self, count=None): + def assert_rate_limited(self, count=None, **headers): """ Asserts that the throttle's rate limit was exceeded and we were denied. """ - response = self._make_requests(count) + response = self._make_requests(count, **headers) assert response.status_code == 429 def test_rate_limiting(self): @@ -84,3 +95,21 @@ def test_staff_with_user_throttle_rate(self): self.user.save() UserThrottleRate.objects.create(user=self.user, rate='10/hour') self.assert_rate_limited(11) + + @ddt.data( + ([], True), + (['enterprise_learner:*'], False), + (['enterprise_admin:*'], False), + (['enterprise_openedx_operator:*'], False), + ) + @ddt.unpack + def test_enterprise_user_throttling_with_jwt_authentication(self, jwt_roles, is_rate_limited): + """ Verify enterprise users are throttled at a higher rate. """ + payload = { + 'roles': jwt_roles, + } + headers = self._build_jwt_headers(self.user, payload) + if is_rate_limited: + self.assert_rate_limited(**headers) + else: + self.assert_rate_limit_successfully_exceeded(**headers) diff --git a/course_discovery/apps/core/throttles.py b/course_discovery/apps/core/throttles.py index c638bb3739..0f3f9a9685 100644 --- a/course_discovery/apps/core/throttles.py +++ b/course_discovery/apps/core/throttles.py @@ -1,5 +1,6 @@ """Custom API throttles.""" from django.core.cache import InvalidCacheBackendError, caches +from edx_rest_framework_extensions.auth.jwt.decoder import configured_jwt_decode_handler from rest_framework.throttling import UserRateThrottle from course_discovery.apps.core.models import UserThrottleRate @@ -16,6 +17,19 @@ def throttling_cache(): return caches['default'] +def is_enterprise_user(request): + """ + Determine whether a JWT-authenticated user is an enterprise user based on the `roles` in + the decoded JWT token associated with the request (e.g., `enterprise_learner`). + """ + jwt_token = request.auth + if not jwt_token: + return False + decoded_jwt = configured_jwt_decode_handler(jwt_token) + roles = decoded_jwt.get('roles', []) + return any('enterprise' in role for role in roles) + + class OverridableUserRateThrottle(UserRateThrottle): """Rate throttling of requests, overridable on a per-user basis.""" cache = throttling_cache() @@ -28,10 +42,15 @@ def allow_request(self, request, view): # Override this throttle's rate if applicable user_throttle = UserThrottleRate.objects.get(user=user) self.rate = user_throttle.rate - self.num_requests, self.duration = self.parse_rate(self.rate) except UserThrottleRate.DoesNotExist: # If we don't have a custom user override, skip throttling if they are a privileged user if user.is_superuser or user.is_staff or is_publisher_user(user): return True + # If the user is not a privileged user, increase throttling rate if they are an enterprise user + if is_enterprise_user(request): + self.rate = '300/hour' + + self.num_requests, self.duration = self.parse_rate(self.rate) + return super().allow_request(request, view) From f84fd291628a367f18b570032ff57acd6a2b64d3 Mon Sep 17 00:00:00 2001 From: Ali Nawaz Date: Tue, 3 Dec 2024 14:55:44 +0500 Subject: [PATCH 3/7] chore: change rate --- course_discovery/apps/core/throttles.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/course_discovery/apps/core/throttles.py b/course_discovery/apps/core/throttles.py index 0f3f9a9685..fb835f4268 100644 --- a/course_discovery/apps/core/throttles.py +++ b/course_discovery/apps/core/throttles.py @@ -1,5 +1,6 @@ """Custom API throttles.""" from django.core.cache import InvalidCacheBackendError, caches +from django.conf import settings from edx_rest_framework_extensions.auth.jwt.decoder import configured_jwt_decode_handler from rest_framework.throttling import UserRateThrottle @@ -49,7 +50,7 @@ def allow_request(self, request, view): # If the user is not a privileged user, increase throttling rate if they are an enterprise user if is_enterprise_user(request): - self.rate = '300/hour' + self.rate = '400/hour' self.num_requests, self.duration = self.parse_rate(self.rate) From 73965ee4cfa7adc85dc9904428ec75f6fbfb57ae Mon Sep 17 00:00:00 2001 From: Ali Nawaz Date: Tue, 3 Dec 2024 17:26:10 +0500 Subject: [PATCH 4/7] refactor: move some constants to settings --- course_discovery/apps/core/tests/test_throttles.py | 3 ++- course_discovery/apps/core/throttles.py | 8 ++++++-- course_discovery/settings/base.py | 5 +++++ 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/course_discovery/apps/core/tests/test_throttles.py b/course_discovery/apps/core/tests/test_throttles.py index 7c3b662c9d..bc553b583b 100644 --- a/course_discovery/apps/core/tests/test_throttles.py +++ b/course_discovery/apps/core/tests/test_throttles.py @@ -112,4 +112,5 @@ def test_enterprise_user_throttling_with_jwt_authentication(self, jwt_roles, is_ if is_rate_limited: self.assert_rate_limited(**headers) else: - self.assert_rate_limit_successfully_exceeded(**headers) + self.assert_rate_limit_successfully_exceeded(count=395, **headers) + self.assert_rate_limited(**headers) diff --git a/course_discovery/apps/core/throttles.py b/course_discovery/apps/core/throttles.py index fb835f4268..a078355560 100644 --- a/course_discovery/apps/core/throttles.py +++ b/course_discovery/apps/core/throttles.py @@ -28,7 +28,11 @@ def is_enterprise_user(request): return False decoded_jwt = configured_jwt_decode_handler(jwt_token) roles = decoded_jwt.get('roles', []) - return any('enterprise' in role for role in roles) + return any( + privileged_role_keyword in role + for privileged_role_keyword in settings.ENHANCED_THROTTLE_JWT_ROLE_KEYWORDS + for role in roles + ) class OverridableUserRateThrottle(UserRateThrottle): @@ -50,7 +54,7 @@ def allow_request(self, request, view): # If the user is not a privileged user, increase throttling rate if they are an enterprise user if is_enterprise_user(request): - self.rate = '400/hour' + self.rate = settings.ENHANCED_THROTTLE_LIMIT self.num_requests, self.duration = self.parse_rate(self.rate) diff --git a/course_discovery/settings/base.py b/course_discovery/settings/base.py index fd45c3244a..b694f07d22 100644 --- a/course_discovery/settings/base.py +++ b/course_discovery/settings/base.py @@ -790,3 +790,8 @@ ) ENABLE_COURSE_REVIEWS_ADMIN = False + +# Keywords that will be searched for inside the `roles` key of the JWT in case a user uses JWT authentication. +# If the keyword is found, the user has more lenient throttling limits. +ENHANCED_THROTTLE_JWT_ROLE_KEYWORDS = ['enterprise'] +ENHANCED_THROTTLE_LIMIT = '400/hour' From 9ac8d7a80ea31191d4cad8081c989af7a0272edc Mon Sep 17 00:00:00 2001 From: Ali Nawaz Date: Tue, 3 Dec 2024 20:26:22 +0500 Subject: [PATCH 5/7] refactor: address reviewer comments --- course_discovery/apps/core/throttles.py | 10 +++++----- course_discovery/settings/base.py | 2 +- course_discovery/settings/test.py | 2 ++ docs/decisions/0029-jwt-based-throttling.rst | 4 ++++ 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/course_discovery/apps/core/throttles.py b/course_discovery/apps/core/throttles.py index a078355560..9ef7f18340 100644 --- a/course_discovery/apps/core/throttles.py +++ b/course_discovery/apps/core/throttles.py @@ -18,10 +18,10 @@ def throttling_cache(): return caches['default'] -def is_enterprise_user(request): +def is_priviledged_user_with_enhanced_throttle(request): """ - Determine whether a JWT-authenticated user is an enterprise user based on the `roles` in - the decoded JWT token associated with the request (e.g., `enterprise_learner`). + Determine whether a JWT-authenticated user has increased throttling rates + based on the `roles` in the decoded JWT token associated with the request. """ jwt_token = request.auth if not jwt_token: @@ -52,8 +52,8 @@ def allow_request(self, request, view): if user.is_superuser or user.is_staff or is_publisher_user(user): return True - # If the user is not a privileged user, increase throttling rate if they are an enterprise user - if is_enterprise_user(request): + # If the user has privileged throttling limits, increase the rate + if is_priviledged_user_with_enhanced_throttle(request): self.rate = settings.ENHANCED_THROTTLE_LIMIT self.num_requests, self.duration = self.parse_rate(self.rate) diff --git a/course_discovery/settings/base.py b/course_discovery/settings/base.py index b694f07d22..76c79a6ac2 100644 --- a/course_discovery/settings/base.py +++ b/course_discovery/settings/base.py @@ -793,5 +793,5 @@ # Keywords that will be searched for inside the `roles` key of the JWT in case a user uses JWT authentication. # If the keyword is found, the user has more lenient throttling limits. -ENHANCED_THROTTLE_JWT_ROLE_KEYWORDS = ['enterprise'] +ENHANCED_THROTTLE_JWT_ROLE_KEYWORDS = [] ENHANCED_THROTTLE_LIMIT = '400/hour' diff --git a/course_discovery/settings/test.py b/course_discovery/settings/test.py index 9994d1ae43..4e6755cab2 100644 --- a/course_discovery/settings/test.py +++ b/course_discovery/settings/test.py @@ -161,3 +161,5 @@ ELASTICSEARCH_DSL_QUERYSET_PAGINATION = 10000 ELASTICSEARCH_DSL_LOAD_PER_QUERY = 10000 + +ENHANCED_THROTTLE_JWT_ROLE_KEYWORDS = ['enterprise'] diff --git a/docs/decisions/0029-jwt-based-throttling.rst b/docs/decisions/0029-jwt-based-throttling.rst index 39c4cc79a2..b5d02bde7d 100644 --- a/docs/decisions/0029-jwt-based-throttling.rst +++ b/docs/decisions/0029-jwt-based-throttling.rst @@ -21,6 +21,10 @@ Since these learners authenticate with Discovery by using JWT tokens issued by t in the JWT to identify them. Enterprise customers are guaranteed to have one of a small number of fixed roles assigned to them. Once we identify that an incoming request's user has one of these roles in their JWT, we will enable higher rate limits for them. +Two new settings, `ENHANCED_THROTTLE_JWT_ROLE_KEYWORDS` AND `ENHANCED_THROTTLE_LIMIT` will be added. The `ENHANCED_THROTTLE_JWT_ROLE_KEYWORDS` +setting accepts a list of strings representing keywords that identify privileged JWT roles. Each keyword is matched against the +roles in a JWT token; if a keyword is found in any role, the user's throttle limits are set to the value of `ENHANCED_THROTTLE_LIMIT`. + Consequences -------------- - Some regular users (i.e without staff or any django group privileges) will have privileged rate limits From d7b74584d994581df3265e85e18f16fe8812561e Mon Sep 17 00:00:00 2001 From: Ali Nawaz Date: Tue, 3 Dec 2024 20:29:23 +0500 Subject: [PATCH 6/7] style: import order --- course_discovery/apps/core/throttles.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/course_discovery/apps/core/throttles.py b/course_discovery/apps/core/throttles.py index 9ef7f18340..76d1875407 100644 --- a/course_discovery/apps/core/throttles.py +++ b/course_discovery/apps/core/throttles.py @@ -1,6 +1,6 @@ """Custom API throttles.""" -from django.core.cache import InvalidCacheBackendError, caches from django.conf import settings +from django.core.cache import InvalidCacheBackendError, caches from edx_rest_framework_extensions.auth.jwt.decoder import configured_jwt_decode_handler from rest_framework.throttling import UserRateThrottle From ba495034a74760d3745c7e85b022b454b7670275 Mon Sep 17 00:00:00 2001 From: Ali Nawaz Date: Tue, 3 Dec 2024 21:09:05 +0500 Subject: [PATCH 7/7] fix: failing tests --- course_discovery/apps/core/tests/test_throttles.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/course_discovery/apps/core/tests/test_throttles.py b/course_discovery/apps/core/tests/test_throttles.py index bc553b583b..9c938121ad 100644 --- a/course_discovery/apps/core/tests/test_throttles.py +++ b/course_discovery/apps/core/tests/test_throttles.py @@ -1,6 +1,7 @@ from unittest.mock import patch import ddt +from django.test.utils import override_settings from django.urls import reverse from rest_framework.test import APITestCase @@ -103,6 +104,7 @@ def test_staff_with_user_throttle_rate(self): (['enterprise_openedx_operator:*'], False), ) @ddt.unpack + @override_settings(ENHANCED_THROTTLE_LIMIT='10/hour') def test_enterprise_user_throttling_with_jwt_authentication(self, jwt_roles, is_rate_limited): """ Verify enterprise users are throttled at a higher rate. """ payload = { @@ -112,5 +114,5 @@ def test_enterprise_user_throttling_with_jwt_authentication(self, jwt_roles, is_ if is_rate_limited: self.assert_rate_limited(**headers) else: - self.assert_rate_limit_successfully_exceeded(count=395, **headers) + self.assert_rate_limit_successfully_exceeded(count=5, **headers) self.assert_rate_limited(**headers)