From 448bb1f61ad546203b914744f9e2052f7c4cc9af Mon Sep 17 00:00:00 2001 From: Tim McCormack Date: Thu, 11 Jan 2024 19:06:00 +0000 Subject: [PATCH] fixup! Fix file uploading and add unit tests - Add dependency on ddt - Report json errors cleanly --- .../tests/test_course_library.zip | Bin 0 -> 238 bytes .../codejail_service/tests/test_views.py | 141 ++++++++++++++++++ .../codejail_service/views.py | 20 ++- requirements/base.txt | 8 +- requirements/dev.txt | 14 +- requirements/doc.txt | 10 +- requirements/quality.txt | 12 +- requirements/scripts.txt | 10 +- requirements/test.in | 1 + requirements/test.txt | 10 +- 10 files changed, 193 insertions(+), 33 deletions(-) create mode 100644 edx_arch_experiments/codejail_service/tests/test_course_library.zip create mode 100644 edx_arch_experiments/codejail_service/tests/test_views.py diff --git a/edx_arch_experiments/codejail_service/tests/test_course_library.zip b/edx_arch_experiments/codejail_service/tests/test_course_library.zip new file mode 100644 index 0000000000000000000000000000000000000000..42409e320b5a13ec3688bb9b6606283099a17bdb GIT binary patch literal 238 zcmWIWW@Zs#U|`^2D4V7oQQqujY6j$)1F;~33`25$X;E=%d`@OkQDRZ0UO{DO2qy#c zev1XE+kv>Wf}4SnzzD=!8bK^94q}Bk2+c78 R-mGjO4U9k-4y3`l834LLK4bs@ literal 0 HcmV?d00001 diff --git a/edx_arch_experiments/codejail_service/tests/test_views.py b/edx_arch_experiments/codejail_service/tests/test_views.py new file mode 100644 index 0000000..0aa005d --- /dev/null +++ b/edx_arch_experiments/codejail_service/tests/test_views.py @@ -0,0 +1,141 @@ +""" +Test codejail service views. +""" + +import json +import textwrap +from os import path + +import ddt +from django.contrib.auth import get_user_model +from django.test import TestCase, override_settings +from django.urls import reverse +from rest_framework.test import APIClient + +from edx_arch_experiments.codejail_service import views + + +@override_settings( + ROOT_URLCONF='edx_arch_experiments.codejail_service.urls', + MIDDLEWARE=[ + 'django.contrib.sessions.middleware.SessionMiddleware', + ], +) +@ddt.ddt +class TestExecService(TestCase): + """Test the v0 code exec view.""" + + def setUp(self): + super().setUp() + user_model = get_user_model() + self.admin_user = user_model.objects.create_user('cms_worker', is_staff=True) + self.other_user = user_model.objects.create_user('student', is_staff=False) + self.standard_params = {'code': 'retval = 3 + 4', 'globals_dict': {}} + + def _test_codejail_api(self, *, user=None, files=None, skip_auth=False, params=None, exp_status, exp_body): + """ + Call the view and make assertions. + + Arguments: + user: User to authenticate as when calling view (None for unauthenticated) + exp_status: Assert that the response HTTP status code is this value + exp_body: Assert that the response body JSON is this value + """ + assert not (user and skip_auth) + + client = APIClient() + user = user or self.admin_user + if not skip_auth: + client.force_authenticate(user) + + params = self.standard_params if params is None else params + payload = json.dumps(params) + req_body = {'payload': payload, **(files or {})} + + resp = client.post(reverse('code_exec_v0'), req_body, format='multipart') + + assert resp.status_code == exp_status + assert json.loads(resp.content) == exp_body + + def test_success(self): + """Regular successful call.""" + self._test_codejail_api( + exp_status=200, exp_body={'globals_dict': {'retval': 7}}, + ) + + @override_settings(CODEJAIL_SERVICE_ENABLED=False) + def test_feature_disabled(self): + """Service can be disabled.""" + self._test_codejail_api( + exp_status=500, exp_body={'error': "Codejail service not enabled"}, + ) + + @override_settings(ENABLE_CODEJAIL_REST_SERVICE=True) + def test_misconfigured_as_relay(self): + """Don't accept codejail requests if we're going to send them elsewhere.""" + self._test_codejail_api( + exp_status=500, exp_body={'error': "Codejail service is misconfigured. (Refusing to act as relay.)"}, + ) + + def test_unauthenticated(self): + """Anonymous requests are rejected.""" + self._test_codejail_api( + skip_auth=True, + exp_status=403, exp_body={'detail': "Authentication credentials were not provided."}, + ) + + def test_unprivileged(self): + """Anonymous requests are rejected.""" + self._test_codejail_api( + user=self.other_user, + exp_status=403, exp_body={'detail': "You do not have permission to perform this action."}, + ) + + def test_unsafely(self): + """unsafely=true is rejected""" + self._test_codejail_api( + params=dict(**self.standard_params, unsafely=True), + exp_status=400, exp_body={'error': "Refusing codejail execution with unsafely=true"}, + ) + + @ddt.unpack + @ddt.data( + ({'globals_dict': {}}, 'code'), + ({'code': 'retval = 3 + 4'}, 'globals_dict'), + ({}, 'code'), + ) + def test_missing_params(self, params, missing): + """code and globals_dict are required""" + self._test_codejail_api( + params=params, + exp_status=400, exp_body={ + 'error': f"Payload JSON did not match schema: '{missing}' is a required property", + }, + ) + + def test_extra_files(self): + # "Course library" containing `course_library.triangular_number`. + # + # It's tempting to use zipfile to write to an io.BytesIO so + # that the test library is in plaintext. Django's request + # factory will indeed see that as a file to use in a multipart + # upload, but it will see it as an empty bytestring. (read() + # returns empty bytestring, while getvalue() returns the + # desired data). So instead we just have a small zip file on + # disk here. + library_path = path.join(path.dirname(__file__), 'test_course_library.zip') + + with open(library_path, 'rb') as lib_zip: + self._test_codejail_api( + params={ + 'code': textwrap.dedent(""" + from course_library import triangular_number + + result = triangular_number(6) + """), + 'globals_dict': {}, + 'python_path': ['python_lib.zip'], + }, + files={'python_lib.zip': lib_zip}, + exp_status=200, exp_body={'globals_dict': {'result': 21}}, + ) diff --git a/edx_arch_experiments/codejail_service/views.py b/edx_arch_experiments/codejail_service/views.py index 44b6c58..a67afa4 100644 --- a/edx_arch_experiments/codejail_service/views.py +++ b/edx_arch_experiments/codejail_service/views.py @@ -6,10 +6,11 @@ import logging from copy import deepcopy -import jsonschema from codejail.safe_exec import SafeExecException, safe_exec from django.conf import settings from edx_toggles.toggles import SettingToggle +from jsonschema.exceptions import best_match as json_error_best_match +from jsonschema.validators import Draft202012Validator from rest_framework.decorators import api_view, parser_classes, permission_classes from rest_framework.parsers import FormParser, MultiPartParser from rest_framework.permissions import IsAdminUser @@ -61,6 +62,11 @@ }, 'required': ['code', 'globals_dict'], } +# Use this rather than jsonschema.validate, since that would check the schema +# every time it is called. Best to do it just once at startup. +Draft202012Validator.check_schema(payload_schema) +payload_validator = Draft202012Validator(payload_schema) + # A note on the authorization model used here: # @@ -126,7 +132,9 @@ def code_exec_view_v0(request): params_json = request.data['payload'] params = json.loads(params_json) - jsonschema.validate(params, payload_schema) + + if json_error := json_error_best_match(payload_validator.iter_errors(params)): + return Response({'error': f"Payload JSON did not match schema: {json_error.message}"}, status=400) complete_code = params['code'] # includes standard prolog input_globals_dict = params['globals_dict'] @@ -135,7 +143,9 @@ def code_exec_view_v0(request): slug = params.get('slug') unsafely = params.get('unsafely') - extra_files = request.FILES + # Convert to a list of (string, bytestring) pairs. Any duplicated file names + # are resolved as last-wins. + extra_files = [(filename, file.read()) for filename, file in request.FILES.items()] # Far too dangerous to allow unsafe executions to come in over the # network, no matter who we think the caller is. The caller is the @@ -154,8 +164,8 @@ def code_exec_view_v0(request): slug=slug, ) except SafeExecException as e: - log.debug("CodejailService execution failed with: {e!r}") + log.debug("CodejailService execution failed for {slug=} with: {e!r}") return Response({'emsg': f"Code jail execution failed: {e!r}"}) - log.debug("CodejailService execution succeeded, with globals={output_globals_dict!r}") + log.debug("CodejailService execution succeeded for {slug=}, with globals={output_globals_dict!r}") return Response({'globals_dict': output_globals_dict}) diff --git a/requirements/base.txt b/requirements/base.txt index 49f7249..9fbaedb 100644 --- a/requirements/base.txt +++ b/requirements/base.txt @@ -61,7 +61,7 @@ edx-django-utils==5.9.0 # -r requirements/base.in # edx-drf-extensions # edx-toggles -edx-drf-extensions==9.0.1 +edx-drf-extensions==9.1.2 # via -r requirements/base.in edx-opaque-keys==2.5.1 # via edx-drf-extensions @@ -73,7 +73,7 @@ importlib-resources==6.1.1 # via # jsonschema # jsonschema-specifications -jinja2==3.1.2 +jinja2==3.1.3 # via code-annotations jsonschema==4.20.0 # via -r requirements/base.in @@ -81,7 +81,7 @@ jsonschema-specifications==2023.12.1 # via jsonschema markupsafe==2.1.3 # via jinja2 -newrelic==9.3.0 +newrelic==9.4.0 # via edx-django-utils pbr==6.0.0 # via stevedore @@ -108,7 +108,7 @@ pytz==2023.3.post1 # djangorestframework pyyaml==6.0.1 # via code-annotations -referencing==0.32.0 +referencing==0.32.1 # via # jsonschema # jsonschema-specifications diff --git a/requirements/dev.txt b/requirements/dev.txt index c6043d6..ca87a77 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -76,6 +76,8 @@ cryptography==41.0.7 # -r requirements/quality.txt # pyjwt # secretstorage +ddt==1.7.1 + # via -r requirements/quality.txt diff-cover==8.0.2 # via -r requirements/dev.in dill==0.3.7 @@ -129,7 +131,7 @@ edx-django-utils==5.9.0 # -r requirements/quality.txt # edx-drf-extensions # edx-toggles -edx-drf-extensions==9.0.1 +edx-drf-extensions==9.1.2 # via -r requirements/quality.txt edx-i18n-tools==1.3.0 # via -r requirements/dev.in @@ -184,7 +186,7 @@ jeepney==0.8.0 # -r requirements/quality.txt # keyring # secretstorage -jinja2==3.1.2 +jinja2==3.1.3 # via # -r requirements/quality.txt # code-annotations @@ -199,7 +201,7 @@ keyring==24.3.0 # via # -r requirements/quality.txt # twine -lxml==5.0.0 +lxml==5.1.0 # via edx-i18n-tools markdown-it-py==3.0.0 # via @@ -217,11 +219,11 @@ mdurl==0.1.2 # via # -r requirements/quality.txt # markdown-it-py -more-itertools==10.1.0 +more-itertools==10.2.0 # via # -r requirements/quality.txt # jaraco-classes -newrelic==9.3.0 +newrelic==9.4.0 # via # -r requirements/quality.txt # edx-django-utils @@ -357,7 +359,7 @@ readme-renderer==42.0 # via # -r requirements/quality.txt # twine -referencing==0.32.0 +referencing==0.32.1 # via # -r requirements/quality.txt # jsonschema diff --git a/requirements/doc.txt b/requirements/doc.txt index 3d06a41..37a1ae6 100644 --- a/requirements/doc.txt +++ b/requirements/doc.txt @@ -48,6 +48,8 @@ cryptography==41.0.7 # via # -r requirements/test.txt # pyjwt +ddt==1.7.1 + # via -r requirements/test.txt django==3.2.23 # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt @@ -94,7 +96,7 @@ edx-django-utils==5.9.0 # -r requirements/test.txt # edx-drf-extensions # edx-toggles -edx-drf-extensions==9.0.1 +edx-drf-extensions==9.1.2 # via -r requirements/test.txt edx-opaque-keys==2.5.1 # via @@ -125,7 +127,7 @@ iniconfig==2.0.0 # via # -r requirements/test.txt # pytest -jinja2==3.1.2 +jinja2==3.1.3 # via # -r requirements/test.txt # code-annotations @@ -140,7 +142,7 @@ markupsafe==2.1.3 # via # -r requirements/test.txt # jinja2 -newrelic==9.3.0 +newrelic==9.4.0 # via # -r requirements/test.txt # edx-django-utils @@ -215,7 +217,7 @@ pyyaml==6.0.1 # code-annotations readme-renderer==42.0 # via -r requirements/doc.in -referencing==0.32.0 +referencing==0.32.1 # via # -r requirements/test.txt # jsonschema diff --git a/requirements/quality.txt b/requirements/quality.txt index a6dd1e6..4a4ef9b 100644 --- a/requirements/quality.txt +++ b/requirements/quality.txt @@ -54,6 +54,8 @@ cryptography==41.0.7 # -r requirements/test.txt # pyjwt # secretstorage +ddt==1.7.1 + # via -r requirements/test.txt dill==0.3.7 # via pylint django==3.2.23 @@ -96,7 +98,7 @@ edx-django-utils==5.9.0 # -r requirements/test.txt # edx-drf-extensions # edx-toggles -edx-drf-extensions==9.0.1 +edx-drf-extensions==9.1.2 # via -r requirements/test.txt edx-lint==5.3.6 # via -r requirements/quality.in @@ -138,7 +140,7 @@ jeepney==0.8.0 # via # keyring # secretstorage -jinja2==3.1.2 +jinja2==3.1.3 # via # -r requirements/test.txt # code-annotations @@ -160,9 +162,9 @@ mccabe==0.7.0 # via pylint mdurl==0.1.2 # via markdown-it-py -more-itertools==10.1.0 +more-itertools==10.2.0 # via jaraco-classes -newrelic==9.3.0 +newrelic==9.4.0 # via # -r requirements/test.txt # edx-django-utils @@ -256,7 +258,7 @@ pyyaml==6.0.1 # code-annotations readme-renderer==42.0 # via twine -referencing==0.32.0 +referencing==0.32.1 # via # -r requirements/test.txt # jsonschema diff --git a/requirements/scripts.txt b/requirements/scripts.txt index 0847847..35ed440 100644 --- a/requirements/scripts.txt +++ b/requirements/scripts.txt @@ -85,7 +85,7 @@ edx-django-utils==5.9.0 # edx-drf-extensions # edx-event-bus-kafka # edx-toggles -edx-drf-extensions==9.0.1 +edx-drf-extensions==9.1.2 # via -r requirements/base.txt edx-event-bus-kafka==5.5.0 # via -r requirements/scripts.in @@ -98,7 +98,7 @@ edx-toggles==5.1.0 # via # -r requirements/base.txt # edx-event-bus-kafka -fastavro==1.9.2 +fastavro==1.9.3 # via # confluent-kafka # openedx-events @@ -111,7 +111,7 @@ importlib-resources==6.1.1 # -r requirements/base.txt # jsonschema # jsonschema-specifications -jinja2==3.1.2 +jinja2==3.1.3 # via # -r requirements/base.txt # code-annotations @@ -125,7 +125,7 @@ markupsafe==2.1.3 # via # -r requirements/base.txt # jinja2 -newrelic==9.3.0 +newrelic==9.4.0 # via # -r requirements/base.txt # edx-django-utils @@ -174,7 +174,7 @@ pyyaml==6.0.1 # via # -r requirements/base.txt # code-annotations -referencing==0.32.0 +referencing==0.32.1 # via # -r requirements/base.txt # jsonschema diff --git a/requirements/test.in b/requirements/test.in index 6797160..f2e6548 100644 --- a/requirements/test.in +++ b/requirements/test.in @@ -6,3 +6,4 @@ pytest-cov # pytest extension for code coverage statistics pytest-django # pytest extension for better Django support code-annotations # provides commands used by the pii_check make target. +ddt # data-driven tests \ No newline at end of file diff --git a/requirements/test.txt b/requirements/test.txt index f88fdf7..86e1fdd 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -44,6 +44,8 @@ cryptography==41.0.7 # via # -r requirements/base.txt # pyjwt +ddt==1.7.1 + # via -r requirements/test.in # via # -c https://raw.githubusercontent.com/edx/edx-lint/master/edx_lint/files/common_constraints.txt # -r requirements/base.txt @@ -81,7 +83,7 @@ edx-django-utils==5.9.0 # -r requirements/base.txt # edx-drf-extensions # edx-toggles -edx-drf-extensions==9.0.1 +edx-drf-extensions==9.1.2 # via -r requirements/base.txt edx-opaque-keys==2.5.1 # via @@ -102,7 +104,7 @@ importlib-resources==6.1.1 # jsonschema-specifications iniconfig==2.0.0 # via pytest -jinja2==3.1.2 +jinja2==3.1.3 # via # -r requirements/base.txt # code-annotations @@ -116,7 +118,7 @@ markupsafe==2.1.3 # via # -r requirements/base.txt # jinja2 -newrelic==9.3.0 +newrelic==9.4.0 # via # -r requirements/base.txt # edx-django-utils @@ -175,7 +177,7 @@ pyyaml==6.0.1 # via # -r requirements/base.txt # code-annotations -referencing==0.32.0 +referencing==0.32.1 # via # -r requirements/base.txt # jsonschema