diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 02d332f..6a638dc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,6 +12,7 @@ jobs: name: Tests runs-on: ${{ matrix.os }} strategy: + fail-fast: false matrix: os: [ ubuntu-latest ] python-version: ['3.12'] @@ -37,7 +38,7 @@ jobs: - name: Run Coverage if: matrix.python-version == '3.12' && matrix.toxenv=='django42-drflatest' - uses: codecov/codecov-action@e79a6962e0d4c0c17b229090214935d2e33f8354 # v6.0.1 + uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f # v7.0.0 with: flags: unittests fail_ci_if_error: true diff --git a/Makefile b/Makefile index e8d0552..fb8f476 100644 --- a/Makefile +++ b/Makefile @@ -83,7 +83,7 @@ diff_cover: test ## Generate diff coverage report diff-cover coverage.xml test_quality: ## Run Quality checks - pylint submissions + DJANGO_SETTINGS_MODULE=settings pylint submissions isort --check-only submissions manage.py setup.py settings.py --skip migrations pycodestyle . --config=pycodestyle diff --git a/settings.py b/settings.py index 07ec8b7..3e9cdf0 100644 --- a/settings.py +++ b/settings.py @@ -2,7 +2,6 @@ Settings for the submissions app. """ - import warnings from django.core.cache import CacheKeyWarning @@ -12,81 +11,77 @@ TEMPLATE_DEBUG = DEBUG DATABASES = { - 'default': { - 'ENGINE': 'django.db.backends.sqlite3', - 'NAME': 'submissions_db', - 'TEST': { - 'NAME': 'submissions_test_db', - } + "default": { + "ENGINE": "django.db.backends.sqlite3", + "NAME": "submissions_db", + "TEST": { + "NAME": "submissions_test_db", + }, }, - 'read_replica': { - 'ENGINE': 'django.db.backends.sqlite3', - 'NAME': 'submissions_read_replica_db', - 'TEST': { - 'MIRROR': 'default', + "read_replica": { + "ENGINE": "django.db.backends.sqlite3", + "NAME": "submissions_read_replica_db", + "TEST": { + "MIRROR": "default", }, }, } # New DB primary keys default to an IntegerField. -DEFAULT_AUTO_FIELD = 'django.db.models.AutoField' +DEFAULT_AUTO_FIELD = "django.db.models.AutoField" CACHES = { - 'default': { - 'BACKEND': 'django.core.cache.backends.locmem.LocMemCache', - 'LOCATION': 'default_loc_mem', + "default": { + "BACKEND": "django.core.cache.backends.locmem.LocMemCache", + "LOCATION": "default_loc_mem", }, } -ROOT_URLCONF = 'urls' +ROOT_URLCONF = "urls" SITE_ID = 1 USE_TZ = True -SECRET_KEY = get_random_string(50, 'abcdefghijklmnopqrstuvwxyz0123456789!@#$%^&*(-_=+)') +SECRET_KEY = get_random_string(50, "abcdefghijklmnopqrstuvwxyz0123456789!@#$%^&*(-_=+)") # Silence cache key warnings # https://docs.djangoproject.com/en/1.4/topics/cache/#cache-key-warnings warnings.simplefilter("ignore", CacheKeyWarning) INSTALLED_APPS = ( - 'django.contrib.auth', - 'django.contrib.contenttypes', - 'django.contrib.sessions', - 'django.contrib.sites', - 'django.contrib.messages', - 'django.contrib.staticfiles', - 'django.contrib.admin', - 'django.contrib.admindocs', - 'release_util', - + "django.contrib.auth", + "django.contrib.contenttypes", + "django.contrib.sessions", + "django.contrib.sites", + "django.contrib.messages", + "django.contrib.staticfiles", + "django.contrib.admin", + "django.contrib.admindocs", + "release_util", # Submissions - 'submissions' + "submissions", ) TEMPLATES = [ { - 'BACKEND': 'django.template.backends.django.DjangoTemplates', - 'OPTIONS': { - 'context_processors': [ - 'django.contrib.auth.context_processors.auth', - 'django.contrib.messages.context_processors.messages', + "BACKEND": "django.template.backends.django.DjangoTemplates", + "OPTIONS": { + "context_processors": [ + "django.contrib.auth.context_processors.auth", + "django.contrib.messages.context_processors.messages", ], }, }, ] MIDDLEWARE = ( - 'django.contrib.sessions.middleware.SessionMiddleware', - 'django.contrib.auth.middleware.AuthenticationMiddleware', - 'django.contrib.messages.middleware.MessageMiddleware' + "django.contrib.sessions.middleware.SessionMiddleware", + "django.middleware.csrf.CsrfViewMiddleware", + "django.contrib.auth.middleware.AuthenticationMiddleware", + "django.contrib.messages.middleware.MessageMiddleware", ) -TEST_APPS = ('submissions',) +TEST_APPS = ("submissions",) EDX_SUBMISSIONS = { - 'MEDIA': { - 'BACKEND': 'django.core.files.storage.InMemoryStorage', - 'OPTIONS': { - } - } + "MEDIA": {"BACKEND": "django.core.files.storage.InMemoryStorage", "OPTIONS": {}} } diff --git a/submissions/__init__.py b/submissions/__init__.py index 319c56c..1c0d0f1 100644 --- a/submissions/__init__.py +++ b/submissions/__init__.py @@ -1,2 +1,2 @@ """ API for creating submissions and scores. """ -__version__ = '4.0.0' +__version__ = '4.0.1' diff --git a/submissions/tests/test_viewsets.py b/submissions/tests/test_viewsets.py index 6b08897..72f2790 100644 --- a/submissions/tests/test_viewsets.py +++ b/submissions/tests/test_viewsets.py @@ -1,6 +1,7 @@ """ Tests for XQueue API views. """ + import json import uuid from datetime import timedelta @@ -13,9 +14,8 @@ from django.urls import reverse from django.utils import timezone from rest_framework import status -from rest_framework.authentication import SessionAuthentication from rest_framework.permissions import AllowAny -from rest_framework.test import APITestCase +from rest_framework.test import APIClient, APITestCase from submissions.models import ExternalGraderDetail, SubmissionFile from submissions.permissions import IsXQueueUser @@ -25,12 +25,7 @@ User = get_user_model() -class CsrfExemptSessionAuthentication(SessionAuthentication): - def enforce_csrf(self, request): - return # Bypass CSRF checks for testing - - -@override_settings(ROOT_URLCONF='submissions.urls') +@override_settings(ROOT_URLCONF="submissions.urls") class TestXqueueViewSet(APITestCase): """ Test cases for XQueueViewSet endpoints. @@ -39,63 +34,54 @@ class TestXqueueViewSet(APITestCase): def setUp(self): """Set up test data.""" super().setUp() - self._orig_auth_classes = XQueueViewSet.authentication_classes - XQueueViewSet.authentication_classes = [CsrfExemptSessionAuthentication] # Ensure the 'xqueue' group exists and add the user to it - xqueue_group, _ = Group.objects.get_or_create(name='xqueue') - self.user = User.objects.create_user( - username='testuser', - password='testpass' - ) + xqueue_group, _ = Group.objects.get_or_create(name="xqueue") + self.user = User.objects.create_user(username="testuser", password="testpass") self.user.groups.add(xqueue_group) self.user.save() self.submission = SubmissionFactory() self.external_grader = ExternalGraderDetailFactory( submission=self.submission, - pullkey='test_pull_key', - status='pending', - queue_name='test_queue', - num_failures=0 + pullkey="test_pull_key", + status="pending", + queue_name="test_queue", + num_failures=0, ) - self.url_put_result = reverse('xqueue-put_result') - self.url_login = reverse('xqueue-login') - self.url_logout = reverse('xqueue-logout') - self.url_status = reverse('xqueue-status') - self.get_submission_url = reverse('xqueue-get_submission') + self.url_put_result = reverse("xqueue-put_result") + self.url_login = reverse("xqueue-login") + self.url_logout = reverse("xqueue-logout") + self.url_status = reverse("xqueue-status") + self.get_submission_url = reverse("xqueue-get_submission") self.viewset = XQueueViewSet() - def tearDown(self): - XQueueViewSet.authentication_classes = self._orig_auth_classes - super().tearDown() - def api_login(self): """Helper to login via the API and set session cookie for subsequent requests.""" - data = {'username': 'testuser', 'password': 'testpass'} + data = {"username": "testuser", "password": "testpass"} response = self.client.post(self.url_login, data) self.assertEqual(response.status_code, status.HTTP_200_OK) # Set sessionid cookie for subsequent requests - if 'sessionid' in response.cookies: - self.client.cookies['sessionid'] = response.cookies['sessionid'].value + if "sessionid" in response.cookies: + self.client.cookies["sessionid"] = response.cookies["sessionid"].value def test_get_permissions_login(self): """Test permissions for login endpoint""" viewset = XQueueViewSet() - viewset.action = 'login' + viewset.action = "login" permissions = viewset.get_permissions() self.assertTrue(any(isinstance(p, AllowAny) for p in permissions)) def test_get_permissions_other_actions(self): """Test permissions for non-login endpoints""" viewset = XQueueViewSet() - viewset.action = 'logout' + viewset.action = "logout" permissions = viewset.get_permissions() self.assertTrue(all(isinstance(p, IsXQueueUser) for p in permissions)) def test_dispatch_valid_session(self): """Test dispatch with valid session""" - self.client.login(username='testuser', password='testpass') + self.client.login(username="testuser", password="testpass") response = self.client.post(self.url_status) self.assertEqual(response.status_code, status.HTTP_200_OK) @@ -107,304 +93,299 @@ def test_dispatch_invalid_session(self): def test_get_submission_missing_queue_name(self): """Test error when queue_name parameter is missing.""" - self.client.login(username='testuser', password='testpass') + self.client.login(username="testuser", password="testpass") response = self.client.get(self.get_submission_url) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) self.assertEqual( response.data, - self.viewset.compose_reply(False, "'get_submission' must provide parameter 'queue_name'") + self.viewset.compose_reply( + False, "'get_submission' must provide parameter 'queue_name'" + ), ) def test_get_submission_queue_empty(self): """Test error when the specified queue is empty.""" - queue_name = 'empty_queue' - self.client.login(username='testuser', password='testpass') + queue_name = "empty_queue" + self.client.login(username="testuser", password="testpass") ExternalGraderDetail.objects.filter(queue_name=queue_name).delete() - response = self.client.get(self.get_submission_url, {'queue_name': queue_name}) + response = self.client.get(self.get_submission_url, {"queue_name": queue_name}) self.assertEqual(response.status_code, status.HTTP_200_OK) # Cambiado de 404 self.assertEqual( response.data, - self.viewset.compose_reply(False, f"Queue '{queue_name}' is empty") + self.viewset.compose_reply(False, f"Queue '{queue_name}' is empty"), ) def test_get_submission_success(self): """Test successfully retrieving a submission from the queue.""" - queue_name = 'prueba' - self.client.login(username='testuser', password='testpass') + queue_name = "prueba" + self.client.login(username="testuser", password="testpass") - initial_pullkey = 'initial_pullkey' + initial_pullkey = "initial_pullkey" new_submission = SubmissionFactory() external_grader = ExternalGraderDetailFactory( queue_name=queue_name, - status='pending', + status="pending", submission=new_submission, - pullkey=initial_pullkey + pullkey=initial_pullkey, ) - response = self.client.get(self.get_submission_url, {'queue_name': queue_name}) + response = self.client.get(self.get_submission_url, {"queue_name": queue_name}) self.assertEqual(response.status_code, status.HTTP_200_OK) - content = json.loads(response.data['content']) - self.assertEqual(response.data['return_code'], 0) + content = json.loads(response.data["content"]) + self.assertEqual(response.data["return_code"], 0) - xqueue_header = json.loads(content['xqueue_header']) - xqueue_body = json.loads(content['xqueue_body']) - self.assertEqual(xqueue_header['submission_id'], new_submission.id) - response_pullkey = xqueue_header['submission_key'] - self.assertEqual(xqueue_body['student_response'], new_submission.answer) - self.assertEqual(content['xqueue_files'], '{}') + xqueue_header = json.loads(content["xqueue_header"]) + xqueue_body = json.loads(content["xqueue_body"]) + self.assertEqual(xqueue_header["submission_id"], new_submission.id) + response_pullkey = xqueue_header["submission_key"] + self.assertEqual(xqueue_body["student_response"], new_submission.answer) + self.assertEqual(content["xqueue_files"], "{}") external_grader.refresh_from_db() - self.assertEqual(external_grader.status, 'pulled') + self.assertEqual(external_grader.status, "pulled") self.assertEqual(external_grader.pullkey, response_pullkey) self.assertIsNotNone(external_grader.status_time) def test_put_result_invalid_submission_id(self): """Test put_result with non-existent submission ID.""" - self.client.login(username='testuser', password='testpass') + self.client.login(username="testuser", password="testpass") response = self.client.post(self.url_status) self.assertEqual(response.status_code, status.HTTP_200_OK) payload = { - 'xqueue_header': json.dumps({ - 'submission_id': 99999, - 'submission_key': 'test_pull_key' - }), - 'xqueue_body': json.dumps({'score': 1}) # Cambiado a entero + "xqueue_header": json.dumps( + {"submission_id": 99999, "submission_key": "test_pull_key"} + ), + "xqueue_body": json.dumps({"score": 1}), # Cambiado a entero } - response = self.client.post(self.url_put_result, payload, format='json') + response = self.client.post(self.url_put_result, payload, format="json") self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) response_data = json.loads(response.content) self.assertEqual( response_data, - self.viewset.compose_reply(False, 'Submission does not exist') + self.viewset.compose_reply(False, "Submission does not exist"), ) def test_put_result_invalid_key(self): """Test put_result with incorrect submission key.""" - self.client.login(username='testuser', password='testpass') + self.client.login(username="testuser", password="testpass") response = self.client.post(self.url_status) self.assertEqual(response.status_code, status.HTTP_200_OK) payload = { - 'xqueue_header': json.dumps({ - 'submission_id': self.submission.id, - 'submission_key': 'wrong_key' - }), - 'xqueue_body': json.dumps({'score': 1}) # Cambiado a entero + "xqueue_header": json.dumps( + {"submission_id": self.submission.id, "submission_key": "wrong_key"} + ), + "xqueue_body": json.dumps({"score": 1}), # Cambiado a entero } - response = self.client.post(self.url_put_result, payload, format='json') + response = self.client.post(self.url_put_result, payload, format="json") self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) response_data = json.loads(response.content) self.assertEqual( response_data, - self.viewset.compose_reply(False, 'Incorrect key for submission') + self.viewset.compose_reply(False, "Incorrect key for submission"), ) def test_put_result_invalid_reply_format(self): """Test put_result with invalid reply format to cover lines 191-193.""" - self.client.login(username='testuser', password='testpass') + self.client.login(username="testuser", password="testpass") response = self.client.post(self.url_status) self.assertEqual(response.status_code, status.HTTP_200_OK) invalid_payloads = [ {}, - {'xqueue_header': 'not_json'}, - {'xqueue_header': '{}', 'xqueue_body': 'not_json'}, - {'xqueue_body': '{}'}, - {'xqueue_header': '{}'}, + {"xqueue_header": "not_json"}, + {"xqueue_header": "{}", "xqueue_body": "not_json"}, + {"xqueue_body": "{}"}, + {"xqueue_header": "{}"}, ] for payload in invalid_payloads: - response = self.client.post(self.url_put_result, payload, format='json') + response = self.client.post(self.url_put_result, payload, format="json") self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) response_data = json.loads(response.content) self.assertEqual( response_data, - self.viewset.compose_reply(False, 'Incorrect reply format') + self.viewset.compose_reply(False, "Incorrect reply format"), ) def test_put_result_set_score_failure(self): """ Test put_result handling when set_score fails. """ - data = { - 'username': 'testuser', - 'password': 'testpass' - } + data = {"username": "testuser", "password": "testpass"} response = self.client.post(self.url_login, data) self.assertEqual(response.status_code, status.HTTP_200_OK) response = self.client.post(self.url_status) self.assertEqual(response.status_code, status.HTTP_200_OK) - self.external_grader.update_status('pulled') + self.external_grader.update_status("pulled") payload = { - 'xqueue_header': json.dumps({ - 'submission_id': self.submission.id, - 'submission_key': self.external_grader.pullkey - }), - 'xqueue_body': json.dumps({'score': 1}) + "xqueue_header": json.dumps( + { + "submission_id": self.submission.id, + "submission_key": self.external_grader.pullkey, + } + ), + "xqueue_body": json.dumps({"score": 1}), } - with patch('submissions.views.xqueue.set_score') as mock_set_score: - mock_set_score.side_effect = Exception('Test error') - response = self.client.post(self.url_put_result, payload, format='json') + with patch("submissions.views.xqueue.set_score") as mock_set_score: + mock_set_score.side_effect = Exception("Test error") + response = self.client.post(self.url_put_result, payload, format="json") self.assertEqual(response.status_code, status.HTTP_200_OK) # Verificar que external_grader status cambió a 'failed' self.external_grader.refresh_from_db() self.assertEqual(self.external_grader.num_failures, 1) - self.assertEqual(self.external_grader.status, 'retry') + self.assertEqual(self.external_grader.status, "retry") def test_put_result_set_score_fail_multiple_times(self): """ Test put_result handling when set_score by intentionally failing multiple times. """ - self.client.login(username='testuser', password='testpass') + self.client.login(username="testuser", password="testpass") response = self.client.post(self.url_status) self.assertEqual(response.status_code, status.HTTP_200_OK) - for each in range(MAX_SCORE_UPDATE_RETRIES+1): - with patch('submissions.views.xqueue.set_score') as mock_set_score: - mock_set_score.side_effect = Exception('Test error') # Make it actually fail + for each in range(MAX_SCORE_UPDATE_RETRIES + 1): + with patch("submissions.views.xqueue.set_score") as mock_set_score: + mock_set_score.side_effect = Exception( + "Test error" + ) # Make it actually fail # Ensure the external grader is in the right state for pulling # If it's failed, we need to reset it to pending first, then to pulled - if self.external_grader.status == 'failed': - self.external_grader.update_status('pending') - self.external_grader.update_status('pulled') + if self.external_grader.status == "failed": + self.external_grader.update_status("pending") + self.external_grader.update_status("pulled") payload = { - 'xqueue_header': json.dumps({ - 'submission_id': self.submission.id, - 'submission_key': self.external_grader.pullkey, - }), - 'xqueue_body': json.dumps({'score': 0.8}) + "xqueue_header": json.dumps( + { + "submission_id": self.submission.id, + "submission_key": self.external_grader.pullkey, + } + ), + "xqueue_body": json.dumps({"score": 0.8}), } - response = self.client.post(self.url_put_result, payload, format='json') + response = self.client.post(self.url_put_result, payload, format="json") self.assertEqual(response.status_code, status.HTTP_200_OK) self.external_grader.refresh_from_db() self.assertEqual(self.external_grader.num_failures, each + 1) - self.assertEqual(self.external_grader.status, 'failed') + self.assertEqual(self.external_grader.status, "failed") - @patch('submissions.views.xqueue.log') + @patch("submissions.views.xqueue.log") def test_put_result_success(self, mock_log): """ Test that appropriate logging occurs in various scenarios. """ - self.submission.external_grader_detail.status = 'pulled' + self.submission.external_grader_detail.status = "pulled" self.submission.external_grader_detail.save() self.submission.external_grader_detail.refresh_from_db() - self.assertEqual(self.submission.external_grader_detail.status, 'pulled') + self.assertEqual(self.submission.external_grader_detail.status, "pulled") payload = { - 'xqueue_header': json.dumps({ - 'submission_id': self.submission.id, - 'submission_key': 'test_pull_key' - }), - 'xqueue_body': json.dumps({'score': 1}) # Cambiado a entero + "xqueue_header": json.dumps( + {"submission_id": self.submission.id, "submission_key": "test_pull_key"} + ), + "xqueue_body": json.dumps({"score": 1}), # Cambiado a entero } - with patch('submissions.api.set_score') as mock_set_score: - self.client.login(username='testuser', password='testpass') + with patch("submissions.api.set_score") as mock_set_score: + self.client.login(username="testuser", password="testpass") response = self.client.post(self.url_status) self.assertEqual(response.status_code, status.HTTP_200_OK) mock_set_score.return_value = True - response = self.client.post(self.url_put_result, payload, format='json') + response = self.client.post(self.url_put_result, payload, format="json") submission_context = { - 'submission_id': self.submission.id, - 'course_id': self.submission.student_item.course_id, - 'user_id': self.submission.student_item.student_id, - 'item_id': self.submission.student_item.item_id, - 'queue_name': self.external_grader.queue_name, - 'queue_key': self.external_grader.queue_key, - } + "submission_id": self.submission.id, + "course_id": self.submission.student_item.course_id, + "user_id": self.submission.student_item.student_id, + "item_id": self.submission.student_item.item_id, + "queue_name": self.external_grader.queue_name, + "queue_key": self.external_grader.queue_key, + } mock_log.info.assert_any_call( "Successfully updated submission %(submission_id)s for user %(user_id)s", - submission_context + submission_context, ) response_data = json.loads(response.content) - self.assertEqual( - response_data, - self.viewset.compose_reply(True, '') - ) + self.assertEqual(response_data, self.viewset.compose_reply(True, "")) self.assertEqual(response.status_code, status.HTTP_200_OK) def test_login_success(self): """Test successful login""" - data = { - 'username': 'testuser', - 'password': 'testpass' - } + data = {"username": "testuser", "password": "testpass"} response = self.client.post(self.url_login, data) self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.data['return_code'], 0) - self.assertIn('sessionid', response.cookies) + self.assertEqual(response.data["return_code"], 0) + self.assertIn("sessionid", response.cookies) def test_login_missing_credentials(self): """Test login with missing credentials""" response = self.client.post(self.url_login, {}) self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - self.assertEqual(response.data['return_code'], 1) + self.assertEqual(response.data["return_code"], 1) def test_login_invalid_credentials(self): """Test login with invalid credentials""" - data = { - 'username': 'testuser', - 'password': 'wrongpass' - } + data = {"username": "testuser", "password": "wrongpass"} response = self.client.post(self.url_login, data) self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) - self.assertEqual(response.data['return_code'], 1) + self.assertEqual(response.data["return_code"], 1) def test_logout(self): """Test logout functionality""" - self.client.login(username='testuser', password='testpass') + self.client.login(username="testuser", password="testpass") response = self.client.post(self.url_logout) self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertEqual(response.data['return_code'], 0) + self.assertEqual(response.data["return_code"], 0) def test_status_endpoint(self): """Test status endpoint""" - self.client.login(username='testuser', password='testpass') + self.client.login(username="testuser", password="testpass") response = self.client.post(self.url_status) self.assertEqual(response.status_code, status.HTTP_200_OK) - content = response.content.decode('utf-8') - self.assertIn('return_code', content) - self.assertIn('content', content) + content = response.content.decode("utf-8") + self.assertIn("return_code", content) + self.assertIn("content", content) def test_get_submission_with_files(self): """Test successfully retrieving a submission with attached files from the queue.""" - self.client.login(username='testuser', password='testpass') + self.client.login(username="testuser", password="testpass") - file_content = b'Test file content' + file_content = b"Test file content" submission_file = SubmissionFile.objects.create( external_grader=self.external_grader, - file=ContentFile(file_content, name='test.txt'), - original_filename='test.txt' + file=ContentFile(file_content, name="test.txt"), + original_filename="test.txt", ) - response = self.client.get(self.get_submission_url, {'queue_name': 'test_queue'}) + response = self.client.get( + self.get_submission_url, {"queue_name": "test_queue"} + ) self.assertEqual(response.status_code, status.HTTP_200_OK) - content = json.loads(response.data['content']) + content = json.loads(response.data["content"]) - xqueue_files = json.loads(content['xqueue_files']) - self.assertIn('test.txt', xqueue_files) - expected_url = f'/test_queue/{submission_file.uuid}' - self.assertEqual(xqueue_files['test.txt'], expected_url) + xqueue_files = json.loads(content["xqueue_files"]) + self.assertIn("test.txt", xqueue_files) + expected_url = f"/test_queue/{submission_file.uuid}" + self.assertEqual(xqueue_files["test.txt"], expected_url) def test_get_submission_with_multiple_files(self): """Test retrieving a submission with multiple attached files.""" - self.client.login(username='testuser', password='testpass') + self.client.login(username="testuser", password="testpass") files_data = [ - ('file1.txt', b'Content 1'), - ('file2.txt', b'Content 2'), - ('file3.txt', b'Content 3') + ("file1.txt", b"Content 1"), + ("file2.txt", b"Content 2"), + ("file3.txt", b"Content 3"), ] created_files = [] @@ -412,76 +393,154 @@ def test_get_submission_with_multiple_files(self): submission_file = SubmissionFile.objects.create( external_grader=self.external_grader, file=ContentFile(content, name=filename), - original_filename=filename + original_filename=filename, ) created_files.append(submission_file) - response = self.client.get(self.get_submission_url, {'queue_name': 'test_queue'}) + response = self.client.get( + self.get_submission_url, {"queue_name": "test_queue"} + ) self.assertEqual(response.status_code, status.HTTP_200_OK) - content = json.loads(response.data['content']) - xqueue_files = json.loads(content['xqueue_files']) + content = json.loads(response.data["content"]) + xqueue_files = json.loads(content["xqueue_files"]) self.assertEqual(len(xqueue_files), len(files_data)) for file_obj, (filename, _) in zip(created_files, files_data): self.assertIn(filename, xqueue_files) - expected_url = f'/test_queue/{file_obj.uuid}' + expected_url = f"/test_queue/{file_obj.uuid}" self.assertEqual(xqueue_files[filename], expected_url) def test_get_submission_file_urls_format(self): """Test that file URLs in the response follow the expected format.""" - self.client.login(username='testuser', password='testpass') + self.client.login(username="testuser", password="testpass") test_uuid = uuid.uuid4() _ = SubmissionFile.objects.create( external_grader=self.external_grader, - file=ContentFile(b'content', name='test.txt'), - original_filename='test.txt', - uuid=test_uuid + file=ContentFile(b"content", name="test.txt"), + original_filename="test.txt", + uuid=test_uuid, ) - response = self.client.get(self.get_submission_url, {'queue_name': 'test_queue'}) + response = self.client.get( + self.get_submission_url, {"queue_name": "test_queue"} + ) self.assertEqual(response.status_code, status.HTTP_200_OK) - content = json.loads(response.data['content']) - xqueue_files = json.loads(content['xqueue_files']) + content = json.loads(response.data["content"]) + xqueue_files = json.loads(content["xqueue_files"]) - expected_url = f'/test_queue/{test_uuid}' - self.assertEqual(xqueue_files['test.txt'], expected_url) + expected_url = f"/test_queue/{test_uuid}" + self.assertEqual(xqueue_files["test.txt"], expected_url) def test_get_submission_already_pulled(self): """Test retrieving a submission that already has a 'pulled' status.""" - queue_name = 'test_queue' - self.client.login(username='testuser', password='testpass') - self.external_grader.update_status('pulled') + queue_name = "test_queue" + self.client.login(username="testuser", password="testpass") + self.external_grader.update_status("pulled") self.external_grader.status_time = timezone.now() - timedelta(minutes=6) self.external_grader.save() - response = self.client.get(self.get_submission_url, {'queue_name': queue_name}) + response = self.client.get(self.get_submission_url, {"queue_name": queue_name}) self.assertEqual(response.status_code, status.HTTP_200_OK) - content = json.loads(response.data['content']) + content = json.loads(response.data["content"]) - xqueue_header = json.loads(content['xqueue_header']) - self.assertEqual(xqueue_header['submission_id'], self.external_grader.submission.id) - self.assertEqual(xqueue_header['submission_key'], self.external_grader.pullkey) + xqueue_header = json.loads(content["xqueue_header"]) + self.assertEqual( + xqueue_header["submission_id"], self.external_grader.submission.id + ) + self.assertEqual(xqueue_header["submission_key"], self.external_grader.pullkey) self.external_grader.refresh_from_db() - self.assertEqual(self.external_grader.status, 'pulled') + self.assertEqual(self.external_grader.status, "pulled") - @patch('submissions.views.xqueue.get_files_for_grader') + @patch("submissions.views.xqueue.get_files_for_grader") def test_get_submission_value_error(self, mock_get_files): """Test get_submission when processing raises ValueError.""" - queue_name = 'test_queue' - self.client.login(username='testuser', password='testpass') + queue_name = "test_queue" + self.client.login(username="testuser", password="testpass") # Use the existing external grader from setUp and update its queue_name and status self.external_grader.queue_name = queue_name - self.external_grader.status = 'pending' + self.external_grader.status = "pending" self.external_grader.save() # Mock get_files_for_grader to raise ValueError to test the exception handler mock_get_files.side_effect = ValueError("File processing error") - response = self.client.get(self.get_submission_url, {'queue_name': queue_name}) + response = self.client.get(self.get_submission_url, {"queue_name": queue_name}) self.assertEqual(response.status_code, status.HTTP_500_INTERNAL_SERVER_ERROR) - self.assertEqual(response.data['return_code'], 1) - self.assertIn("Unable to serialize submission payload", response.data['content']) + self.assertEqual(response.data["return_code"], 1) + self.assertIn( + "Unable to serialize submission payload", response.data["content"] + ) + + def test_login_get_sets_csrf_cookie(self): + """ + Verify that a GET to the login endpoint returns a csrftoken cookie. + + xqueue-watcher pre-fetches the login URL before POSTing credentials so + that it has a CSRF token to include in subsequent mutating requests. + Previously the endpoint only accepted POST, so the GET returned 405 and + no cookie was set, causing put_result to fail with 403. + """ + response = self.client.get(self.url_login) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertIn("csrftoken", response.cookies) + + @patch("submissions.api.set_score") + def test_xqueue_watcher_csrf_flow(self, _mock_set_score): + """ + Verify the full xqueue-watcher CSRF flow works end-to-end with standard + SessionAuthentication (no CSRF bypass needed). + + Steps: + 1. GET login to obtain the CSRF cookie. + 2. POST credentials to log in (CSRF is not enforced for unauthenticated + requests, so no token is needed here). + 3. POST put_result with session cookie + X-CSRFToken header. + + enforce_csrf_checks=True confirms that step 3 passes only when the + correct CSRF token is present — proving xqueue-watcher can operate + without any CSRF exemption in the production code. + """ + csrf_client = APIClient(enforce_csrf_checks=True) + + # Step 1: GET login to obtain the CSRF cookie. + response = csrf_client.get(self.url_login) + self.assertEqual(response.status_code, status.HTTP_200_OK) + csrf_token = response.cookies["csrftoken"].value + + # Step 2: POST credentials to log in. + response = csrf_client.post( + self.url_login, + {"username": "testuser", "password": "testpass"}, + ) + self.assertEqual(response.status_code, status.HTTP_200_OK) + # django.contrib.auth.login() calls rotate_token(), which issues a new + # CSRF secret. Re-read the token from the login response so that step 3 + # sends the current token, not the now-stale one from step 1. + csrf_token = response.cookies["csrftoken"].value + + # Step 3: POST put_result with session and CSRF token. + self.external_grader.update_status("pulled") + self.external_grader.refresh_from_db() + + payload = { + "xqueue_header": json.dumps( + { + "submission_id": self.submission.id, + "submission_key": self.external_grader.pullkey, + } + ), + "xqueue_body": json.dumps({"score": 1}), + } + + response = csrf_client.post( + self.url_put_result, + payload, + format="json", + HTTP_X_CSRFTOKEN=csrf_token, + ) + + self.assertEqual(response.status_code, status.HTTP_200_OK) diff --git a/submissions/views/xqueue.py b/submissions/views/xqueue.py index f5dc3b5..0041baa 100644 --- a/submissions/views/xqueue.py +++ b/submissions/views/xqueue.py @@ -8,6 +8,7 @@ from django.db import DatabaseError, transaction from django.db.models import Q from django.http import HttpResponse +from django.middleware.csrf import get_token from django.utils import timezone from openedx_events.learning.data import ExternalGraderScoreData from openedx_events.learning.signals import EXTERNAL_GRADER_SCORE_SUBMITTED @@ -59,7 +60,7 @@ class XQueueViewSet(viewsets.ViewSet): For more context, see DEPR: https://github.com/openedx/public-engineering/issues/286. """ - authentication_classes = [SessionAuthentication] # Xqueue watcher auth method + authentication_classes = [SessionAuthentication] def get_permissions(self): """ @@ -67,57 +68,66 @@ def get_permissions(self): - Login endpoint is public - All other endpoints require authentication """ - if self.action == 'login': + if self.action == "login": permission_classes = [AllowAny] else: permission_classes = [IsXQueueUser] return [permission() for permission in permission_classes] - @action(detail=False, methods=['post'], url_name='login') + @action(detail=False, methods=["get", "post"], url_name="login") def login(self, request): """ Endpoint for authenticating users and creating sessions. + + GET: Sets the CSRF cookie so that xqueue-watcher can extract the token + before POSTing credentials. Returns an empty success response. + + POST: Authenticates the supplied username/password and opens a session. """ - if 'username' not in request.data or 'password' not in request.data: - log.error('XQueue insufficient login info') + if request.method == "GET": + # xqueue-watcher pre-fetches this URL to obtain a CSRF token. + # Calling get_token() ensures the csrftoken cookie is included in + # the response; the watcher then sends that value as the + # X-CSRFToken header in subsequent mutating requests. + get_token(request) + return Response(self.compose_reply(True, ""), status=status.HTTP_200_OK) + + if "username" not in request.data or "password" not in request.data: + log.error("XQueue insufficient login info") return Response( - self.compose_reply(False, 'Insufficient login info'), - status=status.HTTP_400_BAD_REQUEST + self.compose_reply(False, "Insufficient login info"), + status=status.HTTP_400_BAD_REQUEST, ) user = authenticate( request, - username=request.data['username'], - password=request.data['password'] + username=request.data["username"], + password=request.data["password"], ) if user is None: - log.error('XQueue username or password incorrect') + log.error("XQueue username or password incorrect") return Response( - self.compose_reply(False, 'Incorrect login credentials'), - status=status.HTTP_401_UNAUTHORIZED + self.compose_reply(False, "Incorrect login credentials"), + status=status.HTTP_401_UNAUTHORIZED, ) login(request, user) - log.info(f'XQueue user logged in {request.user.username}') + log.info(f"XQueue user logged in {request.user.username}") return Response( - self.compose_reply(True, 'Logged in'), - status=status.HTTP_200_OK + self.compose_reply(True, "Logged in"), status=status.HTTP_200_OK ) - @action(detail=False, methods=['post'], url_name='logout') + @action(detail=False, methods=["post"], url_name="logout") def logout(self, request): """ Endpoint for ending user sessions. """ logout(request) - return Response( - self.compose_reply(True, 'Goodbye'), - status=status.HTTP_200_OK - ) + return Response(self.compose_reply(True, "Goodbye"), status=status.HTTP_200_OK) - @action(detail=False, methods=['get'], url_name='get_submission') + @action(detail=False, methods=["get"], url_name="get_submission") @transaction.atomic def get_submission(self, request): """ @@ -128,34 +138,39 @@ def get_submission(self, request): - Submission data with pull information if successful - Error message if queue is empty or invalid """ - queue_name = request.query_params.get('queue_name') + queue_name = request.query_params.get("queue_name") if not queue_name: log.error("'get_submission' must provide parameter 'queue_name'") return Response( - self.compose_reply(False, "'get_submission' must provide parameter 'queue_name'"), - status=status.HTTP_400_BAD_REQUEST + self.compose_reply( + False, "'get_submission' must provide parameter 'queue_name'" + ), + status=status.HTTP_400_BAD_REQUEST, ) timeout_threshold = timezone.now() - timedelta(minutes=5) # DatabaseError handling was removed because with skip_locked=True, locking conflicts # are avoided and this exception is no longer expected here. external_grader = ( - ExternalGraderDetail.objects - .select_for_update(skip_locked=True) + ExternalGraderDetail.objects.select_for_update(skip_locked=True) .filter( - Q(queue_name=queue_name, status='pending') | - Q(queue_name=queue_name, status='retry') | - Q(queue_name=queue_name, status='pulled', status_time__lt=timeout_threshold) + Q(queue_name=queue_name, status="pending") + | Q(queue_name=queue_name, status="retry") + | Q( + queue_name=queue_name, + status="pulled", + status_time__lt=timeout_threshold, + ) ) - .select_related('submission') - .order_by('status_time') + .select_related("submission") + .order_by("status_time") .first() ) if external_grader: submission_id = external_grader.submission.id - if external_grader.status != 'pulled': + if external_grader.status != "pulled": try: external_grader.update_status("pulled") except ValueError as error: @@ -163,78 +178,89 @@ def get_submission(self, request): "Invalid status transition for submission %s: current=%s target='pulled'", submission_id, external_grader.status, - exc_info=error + exc_info=error, ) return Response( - self.compose_reply(False, 'Unable to transition submission for grading'), - status=status.HTTP_409_CONFLICT + self.compose_reply( + False, "Unable to transition submission for grading" + ), + status=status.HTTP_409_CONFLICT, ) try: submission_data = { - "grader_payload": json.dumps({"grader": external_grader.grader_file_name}), - "student_info": json.dumps({ - "anonymous_student_id": str(external_grader.submission.student_item.student_id), - "submission_time": str(int(external_grader.created_at.timestamp())), - "random_seed": 1 - }), - "student_response": external_grader.submission.answer + "grader_payload": json.dumps( + {"grader": external_grader.grader_file_name} + ), + "student_info": json.dumps( + { + "anonymous_student_id": str( + external_grader.submission.student_item.student_id + ), + "submission_time": str( + int(external_grader.created_at.timestamp()) + ), + "random_seed": 1, + } + ), + "student_response": external_grader.submission.answer, } payload = { - 'xqueue_header': json.dumps({ - 'submission_id': submission_id, - 'submission_key': external_grader.pullkey - }), - 'xqueue_body': json.dumps(submission_data), + "xqueue_header": json.dumps( + { + "submission_id": submission_id, + "submission_key": external_grader.pullkey, + } + ), + "xqueue_body": json.dumps(submission_data), # Xqueue watcher expects this to be a JSON string - 'xqueue_files': json.dumps(get_files_for_grader(external_grader)) + "xqueue_files": json.dumps(get_files_for_grader(external_grader)), } except (TypeError, ValueError) as error: log.error( "Unable to serialize submission payload for submission %s: %s", submission_id, - error + error, ) return Response( - self.compose_reply(False, 'Unable to serialize submission payload'), - status=status.HTTP_500_INTERNAL_SERVER_ERROR + self.compose_reply(False, "Unable to serialize submission payload"), + status=status.HTTP_500_INTERNAL_SERVER_ERROR, ) return Response( self.compose_reply(True, content=json.dumps(payload)), - status=status.HTTP_200_OK + status=status.HTTP_200_OK, ) return Response( self.compose_reply(False, f"Queue '{queue_name}' is empty"), - status=status.HTTP_200_OK + status=status.HTTP_200_OK, ) - @action(detail=False, methods=['post'], url_name='put_result') + @action(detail=False, methods=["post"], url_name="put_result") @transaction.atomic def put_result(self, request): """ Endpoint for graders to post their results and update submission scores. """ (reply_valid, submission_id, submission_key, score_msg, points_earned) = ( - self.validate_grader_reply(request.data)) + self.validate_grader_reply(request.data) + ) if not reply_valid: - log.error("Invalid reply from pull-grader: request.data: %s", - request.data) + log.error("Invalid reply from pull-grader: request.data: %s", request.data) return Response( - self.compose_reply(False, 'Incorrect reply format'), - status=status.HTTP_400_BAD_REQUEST + self.compose_reply(False, "Incorrect reply format"), + status=status.HTTP_400_BAD_REQUEST, ) try: # Lock the row to prevent duplicate grading attempts from racing and emitting # duplicate score events for the same submission. external_grader = ( - ExternalGraderDetail.objects - .select_for_update(nowait=True) - .select_related('submission__student_item') + ExternalGraderDetail.objects.select_for_update(nowait=True) + .select_related("submission__student_item") .get(submission__id=submission_id) ) except ExternalGraderDetail.DoesNotExist: @@ -243,20 +269,20 @@ def put_result(self, request): "grader: %s, submission_key: %s, score_msg: %s", submission_id, submission_key, - score_msg + score_msg, ) return Response( - self.compose_reply(False, 'Submission does not exist'), - status=status.HTTP_404_NOT_FOUND + self.compose_reply(False, "Submission does not exist"), + status=status.HTTP_404_NOT_FOUND, ) except DatabaseError: log.warning( "Concurrent grade update detected for submission %s; rejecting duplicate request", - submission_id + submission_id, ) return Response( - self.compose_reply(False, 'Submission is currently being processed'), - status=status.HTTP_409_CONFLICT + self.compose_reply(False, "Submission is currently being processed"), + status=status.HTTP_409_CONFLICT, ) if not external_grader.pullkey or submission_key != external_grader.pullkey: @@ -264,40 +290,40 @@ def put_result(self, request): "Invalid pullkey for submission_id %s: received '%s', expected '%s'", submission_id, submission_key, - external_grader.pullkey + external_grader.pullkey, ) return Response( - self.compose_reply(False, 'Incorrect key for submission'), - status=status.HTTP_403_FORBIDDEN + self.compose_reply(False, "Incorrect key for submission"), + status=status.HTTP_403_FORBIDDEN, ) # pylint: disable=broad-exception-caught submission_context = { - 'submission_id': submission_id, - 'course_id': external_grader.submission.student_item.course_id, - 'user_id': external_grader.submission.student_item.student_id, - 'item_id': external_grader.submission.student_item.item_id, - 'queue_name': external_grader.queue_name, - 'queue_key': external_grader.queue_key, + "submission_id": submission_id, + "course_id": external_grader.submission.student_item.course_id, + "user_id": external_grader.submission.student_item.student_id, + "item_id": external_grader.submission.student_item.item_id, + "queue_name": external_grader.queue_name, + "queue_key": external_grader.queue_key, } try: log.info( "Attempting to record score for submission %(submission_id)s " "(course=%(course_id)s, item=%(item_id)s, user=%(user_id)s)", - submission_context + submission_context, ) set_score( str(external_grader.submission.uuid), points_earned, - external_grader.points_possible + external_grader.points_possible, ) external_grader.grader_reply = score_msg - external_grader.save(update_fields=['grader_reply']) - external_grader.update_status('retired') + external_grader.save(update_fields=["grader_reply"]) + external_grader.update_status("retired") log.info( "Successfully updated submission %(submission_id)s for user %(user_id)s", - submission_context + submission_context, ) EXTERNAL_GRADER_SCORE_SUBMITTED.send_event( @@ -305,35 +331,35 @@ def put_result(self, request): score=ExternalGraderScoreData( points_possible=external_grader.points_possible, points_earned=points_earned, - course_id=submission_context['course_id'], + course_id=submission_context["course_id"], score_msg=score_msg, submission_id=submission_id, - user_id=submission_context['user_id'], - module_id=submission_context['item_id'], - queue_key=submission_context['queue_key'], - queue_name=submission_context['queue_name'] - ) + user_id=submission_context["user_id"], + module_id=submission_context["item_id"], + queue_key=submission_context["queue_key"], + queue_name=submission_context["queue_name"], + ), ) log.info( "External grader score event emitted for submission %(submission_id)s " "(course=%(course_id)s, item=%(item_id)s, user=%(user_id)s, " "queue=%(queue_name)s, queue_key=%(queue_key)s)", - submission_context + submission_context, ) except Exception: log.exception( "Error recording score for submission %(submission_id)s " "(course=%(course_id)s, item=%(item_id)s, user=%(user_id)s, queue=%(queue_name)s)", - submission_context + submission_context, ) # Keep track of how many times we've failed to set_score a grade for this submission if external_grader.num_failures + 1 >= MAX_SCORE_UPDATE_RETRIES: - external_grader.update_status('failed') + external_grader.update_status("failed") else: - external_grader.update_status('retry') + external_grader.update_status("retry") - return Response(self.compose_reply(success=True, content='')) + return Response(self.compose_reply(success=True, content="")) def validate_grader_reply(self, external_reply): """ @@ -342,14 +368,14 @@ def validate_grader_reply(self, external_reply): Returns: tuple: (is_valid, submission_id, submission_key, grader_reply) """ - fail = (False, -1, '', '', '') + fail = (False, -1, "", "", "") if not isinstance(external_reply, dict): return fail try: - header = external_reply['xqueue_header'] - grader_reply = external_reply['xqueue_body'] + header = external_reply["xqueue_header"] + grader_reply = external_reply["xqueue_body"] except KeyError: return fail @@ -362,19 +388,22 @@ def validate_grader_reply(self, external_reply): score = json.loads(grader_reply) points_earned = score.get("score") except (TypeError, ValueError) as e: - log.error("Failed to parse grader_reply as JSON or extract score: %s. Raw grader_reply: %s", - str(e), grader_reply) + log.error( + "Failed to parse grader_reply as JSON or extract score: %s. Raw grader_reply: %s", + str(e), + grader_reply, + ) return fail if not isinstance(header_dict, dict): return fail - for tag in ['submission_id', 'submission_key']: + for tag in ["submission_id", "submission_key"]: if tag not in header_dict: return fail - submission_id = int(header_dict['submission_id']) - submission_key = header_dict['submission_key'] + submission_id = int(header_dict["submission_id"]) + submission_key = header_dict["submission_key"] return (True, submission_id, submission_key, grader_reply, points_earned) @@ -389,11 +418,8 @@ def compose_reply(self, success, content): Returns: dict: Formatted response """ - return { - 'return_code': 0 if success else 1, - 'content': content - } + return {"return_code": 0 if success else 1, "content": content} - @action(detail=False, methods=['post'], url_name='status') + @action(detail=False, methods=["post"], url_name="status") def status(self, _): - return HttpResponse(self.compose_reply(success=True, content='OK')) + return HttpResponse(self.compose_reply(success=True, content="OK"))