Skip to content

Commit

Permalink
fixup! Fix file uploading and add unit tests
Browse files Browse the repository at this point in the history
- Add dependency on ddt
- Report json errors cleanly
  • Loading branch information
timmc-edx committed Jan 11, 2024
1 parent 6c02d0e commit 448bb1f
Show file tree
Hide file tree
Showing 10 changed files with 193 additions and 33 deletions.
Binary file not shown.
141 changes: 141 additions & 0 deletions edx_arch_experiments/codejail_service/tests/test_views.py
Original file line number Diff line number Diff line change
@@ -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}},
)
20 changes: 15 additions & 5 deletions edx_arch_experiments/codejail_service/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
#
Expand Down Expand Up @@ -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']
Expand All @@ -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
Expand All @@ -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})
8 changes: 4 additions & 4 deletions requirements/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -73,15 +73,15 @@ 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
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
Expand All @@ -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
Expand Down
14 changes: 8 additions & 6 deletions requirements/dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
10 changes: 6 additions & 4 deletions requirements/doc.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
12 changes: 7 additions & 5 deletions requirements/quality.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
Loading

0 comments on commit 448bb1f

Please sign in to comment.