From cd3e4353b1fd09ab545623b5de28c42cd4f699f8 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 4 Feb 2021 11:11:07 -0500 Subject: [PATCH 1/2] feat: Add a 403 and 429 handler. See context here: https://django-ratelimit.readthedocs.io/en/latest/cookbook/429.html#context For now we continue to fall back to django's default 403 handler for 403 but provide a new 429 template that we use for ratelimit exceptions. This commit also updates a logistration test that relied on the old 403 behavior of django-ratelimit instead of the newly added 429 behavior. --- lms/djangoapps/instructor/tests/test_api.py | 2 +- lms/djangoapps/static_template_view/views.py | 24 ++++++++++++++++- lms/templates/static_templates/429.html | 26 +++++++++++++++++++ lms/urls.py | 1 + .../views/tests/test_logistration.py | 2 +- 5 files changed, 52 insertions(+), 3 deletions(-) create mode 100644 lms/templates/static_templates/429.html diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index db2b0d1d70..02584c39a5 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -2805,7 +2805,7 @@ class TestInstructorAPILevelsDataDump(SharedModuleStoreTestCase, LoginEnrollment # other users of the lms. with freeze_time(base_time + datetime.timedelta(minutes=1)): response = self.client.post(url, {}) - assert response.status_code == 403 + assert response.status_code == 429 with freeze_time(base_time + datetime.timedelta(minutes=5)): response = self.client.post(url, {}) diff --git a/lms/djangoapps/static_template_view/views.py b/lms/djangoapps/static_template_view/views.py index ffe3a31338..9ce2666bc2 100644 --- a/lms/djangoapps/static_template_view/views.py +++ b/lms/djangoapps/static_template_view/views.py @@ -9,11 +9,13 @@ import mimetypes from django.conf import settings -from django.http import Http404, HttpResponseNotFound, HttpResponseServerError +from django.http import Http404, HttpResponse, HttpResponseNotFound, HttpResponseServerError from django.shortcuts import redirect from django.template import TemplateDoesNotExist from django.utils.safestring import mark_safe from django.views.decorators.csrf import ensure_csrf_cookie +from django.views.defaults import permission_denied +from ratelimit.exceptions import Ratelimited from mako.exceptions import TopLevelLookupException from common.djangoapps.edxmako.shortcuts import render_to_response, render_to_string @@ -92,12 +94,32 @@ def render_press_release(request, slug): return resp +@fix_crum_request +def render_403(request, exception=None): + """ + Render the permission_denied template unless it's a ratelimit exception in which case use the rate limit template. + """ + if isinstance(exception, Ratelimited): + return render_429(request, exception) + + return permission_denied(request, exception) + + @fix_crum_request def render_404(request, exception): # lint-amnesty, pylint: disable=unused-argument request.view_name = '404' return HttpResponseNotFound(render_to_string('static_templates/404.html', {}, request=request)) +@fix_crum_request +def render_429(request, exception=None): # lint-amnesty, pylint: disable=unused-argument + """ + Render the rate limit template as an HttpResponse. + """ + request.view_name = '429' + return HttpResponse(render_to_string('static_templates/429.html', {}, request=request), status=429) + + @fix_crum_request def render_500(request): return HttpResponseServerError(render_to_string('static_templates/server-error.html', {}, request=request)) diff --git a/lms/templates/static_templates/429.html b/lms/templates/static_templates/429.html new file mode 100644 index 0000000000..8f3079f02f --- /dev/null +++ b/lms/templates/static_templates/429.html @@ -0,0 +1,26 @@ +<%page expression_filter="h"/> +<%namespace name='static' file='../static_content.html'/> +<%! +from django.utils.translation import ugettext as _ +from openedx.core.djangolib.markup import HTML, Text +%> +<%inherit file="../main.html" /> + +<%block name="pagetitle">${_("Too Many Requests")} + +
+
+

+ <%block name="pageheader">${page_header or _("Too Many Requests")} +

+

+ <%block name="pagecontent"> + % if page_content: + ${page_content} + % else: + ${Text(_('Your request has been rate-limited. Please try again later.'))} + % endif + +

+
+
diff --git a/lms/urls.py b/lms/urls.py index 24c6e204b8..f3fd44c15b 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -69,6 +69,7 @@ if settings.DEBUG or settings.FEATURES.get('ENABLE_DJANGO_ADMIN_SITE'): # Custom error pages # These are used by Django to render these error codes. Do not remove. # pylint: disable=invalid-name +handler403 = static_template_view_views.render_403 handler404 = static_template_view_views.render_404 handler500 = static_template_view_views.render_500 diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py b/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py index a2b50573ce..cf74f1ed23 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py @@ -137,7 +137,7 @@ class LoginAndRegistrationTest(ThirdPartyAuthTestMixin, UrlResetMixin, ModuleSto # then the rate limiter should kick in and give a HttpForbidden response response = self.client.get(login_url) - self.assertEqual(response.status_code, 403) + self.assertEqual(response.status_code, 429) # now reset the time to 6 mins from now in future in order to unblock reset_time = datetime.now(UTC) + timedelta(seconds=361) From cfca652deecb3e46fcbdec822af43b0c6735c8c7 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 4 Feb 2021 11:14:22 -0500 Subject: [PATCH 2/2] feat: Add paths to easily view various error pages. Currently it's hard to see the content of an error without knowing how to cause an existing view to make that error in production. Adding these default paths should make that a lot easier. --- lms/urls.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lms/urls.py b/lms/urls.py index f3fd44c15b..5e9f0a523b 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -7,6 +7,7 @@ from django.conf import settings from django.conf.urls import include, url from django.conf.urls.static import static from django.contrib.admin import autodiscover as django_autodiscover +from django.urls import path from django.utils.translation import ugettext_lazy as _ from django.views.generic.base import RedirectView from edx_api_doc_tools import make_docs_urls @@ -71,6 +72,7 @@ if settings.DEBUG or settings.FEATURES.get('ENABLE_DJANGO_ADMIN_SITE'): # pylint: disable=invalid-name handler403 = static_template_view_views.render_403 handler404 = static_template_view_views.render_404 +handler429 = static_template_view_views.render_429 handler500 = static_template_view_views.render_500 notification_prefs_urls = [ @@ -199,6 +201,10 @@ urlpatterns = [ ), url(r'^api/discounts/', include(('openedx.features.discounts.urls', 'openedx.features.discounts'), namespace='api_discounts')), + path('403', handler403), + path('404', handler404), + path('429', handler429), + path('500', handler500), ] if settings.FEATURES.get('ENABLE_MOBILE_REST_API'):