From 7c727cb57ad8d4705a4461014a8a276dbe3e485b Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Tue, 26 May 2026 12:07:14 -0400 Subject: [PATCH 1/8] fix: use CsrfExemptSessionAuthentication in XQueueViewSet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The XQueue API is a service-to-service interface used by xqueue-watcher. When the watcher POSTs to put_result with a valid session, DRF's SessionAuthentication enforces CSRF for authenticated requests. The watcher has no reliable way to obtain a CSRF token (the login endpoint only accepts POST, so the pre-login GET returns 405 with no cookie set). Extract CsrfExemptSessionAuthentication into submissions/authentication.py and use it in XQueueViewSet. CSRF is not appropriate for this API; authorization is already enforced by the IsXQueueUser permission class. The test module previously defined its own local copy of this class for test setup — update it to import from the production module instead. --- submissions/authentication.py | 17 +++++++++++++++++ submissions/tests/test_viewsets.py | 7 +------ submissions/views/xqueue.py | 4 ++-- 3 files changed, 20 insertions(+), 8 deletions(-) create mode 100644 submissions/authentication.py diff --git a/submissions/authentication.py b/submissions/authentication.py new file mode 100644 index 0000000..f01e9f8 --- /dev/null +++ b/submissions/authentication.py @@ -0,0 +1,17 @@ +"""Custom authentication classes for the submissions app.""" + +from rest_framework.authentication import SessionAuthentication + + +class CsrfExemptSessionAuthentication(SessionAuthentication): + """ + Session authentication that skips CSRF enforcement. + + The XQueue API is a service-to-service interface used by xqueue-watcher. + It is not a browser-facing endpoint, so CSRF protection is not applicable. + Authorization is handled instead by the IsXQueueUser permission class, which + requires the authenticated user to be a member of the 'xqueue' group. + """ + + def enforce_csrf(self, request): + return # no-op: CSRF not required for service-to-service API calls diff --git a/submissions/tests/test_viewsets.py b/submissions/tests/test_viewsets.py index 6b08897..07ad3e3 100644 --- a/submissions/tests/test_viewsets.py +++ b/submissions/tests/test_viewsets.py @@ -13,10 +13,10 @@ 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 submissions.authentication import CsrfExemptSessionAuthentication from submissions.models import ExternalGraderDetail, SubmissionFile from submissions.permissions import IsXQueueUser from submissions.tests.factories import ExternalGraderDetailFactory, SubmissionFactory @@ -25,11 +25,6 @@ User = get_user_model() -class CsrfExemptSessionAuthentication(SessionAuthentication): - def enforce_csrf(self, request): - return # Bypass CSRF checks for testing - - @override_settings(ROOT_URLCONF='submissions.urls') class TestXqueueViewSet(APITestCase): """ diff --git a/submissions/views/xqueue.py b/submissions/views/xqueue.py index f5dc3b5..c3f685c 100644 --- a/submissions/views/xqueue.py +++ b/submissions/views/xqueue.py @@ -12,12 +12,12 @@ from openedx_events.learning.data import ExternalGraderScoreData from openedx_events.learning.signals import EXTERNAL_GRADER_SCORE_SUBMITTED from rest_framework import status, viewsets -from rest_framework.authentication import SessionAuthentication from rest_framework.decorators import action from rest_framework.permissions import AllowAny from rest_framework.response import Response from submissions.api import get_files_for_grader, set_score +from submissions.authentication import CsrfExemptSessionAuthentication from submissions.models import ExternalGraderDetail from submissions.permissions import IsXQueueUser @@ -59,7 +59,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 = [CsrfExemptSessionAuthentication] def get_permissions(self): """ From 30c5378a52da18f50e9e6c487d81c4217c641ac3 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Tue, 26 May 2026 12:21:33 -0400 Subject: [PATCH 2/8] fix: address PR review feedback on CsrfExemptSessionAuthentication - Expand docstring to document the security constraint: the 'xqueue' group must be restricted to dedicated service accounts and never granted to regular browser-accessible user accounts, as doing so would reintroduce a CSRF attack surface on the state-changing endpoints (put_result, logout). - Add test_put_result_succeeds_without_csrf_token which uses APIClient(enforce_csrf_checks=True) to exercise the actual DRF-level CSRF enforcement path, confirming that CsrfExemptSessionAuthentication bypasses it and that put_result returns 200 without a CSRF token. --- submissions/authentication.py | 7 +++++++ submissions/tests/test_viewsets.py | 30 +++++++++++++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/submissions/authentication.py b/submissions/authentication.py index f01e9f8..d2bf815 100644 --- a/submissions/authentication.py +++ b/submissions/authentication.py @@ -11,6 +11,13 @@ class CsrfExemptSessionAuthentication(SessionAuthentication): It is not a browser-facing endpoint, so CSRF protection is not applicable. Authorization is handled instead by the IsXQueueUser permission class, which requires the authenticated user to be a member of the 'xqueue' group. + + Security note: because this class disables CSRF while retaining cookie-based + session auth, it is important that the 'xqueue' group is restricted strictly + to dedicated service accounts and never granted to regular user accounts that + may also authenticate via a browser. Granting the 'xqueue' group to a + browser-accessible account would reintroduce a CSRF attack surface against + the state-changing endpoints (put_result, logout) of this API. """ def enforce_csrf(self, request): diff --git a/submissions/tests/test_viewsets.py b/submissions/tests/test_viewsets.py index 07ad3e3..0718fe1 100644 --- a/submissions/tests/test_viewsets.py +++ b/submissions/tests/test_viewsets.py @@ -14,7 +14,7 @@ from django.utils import timezone from rest_framework import status from rest_framework.permissions import AllowAny -from rest_framework.test import APITestCase +from rest_framework.test import APIClient, APITestCase from submissions.authentication import CsrfExemptSessionAuthentication from submissions.models import ExternalGraderDetail, SubmissionFile @@ -480,3 +480,31 @@ def test_get_submission_value_error(self, mock_get_files): 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']) + + @patch('submissions.api.set_score') + def test_put_result_succeeds_without_csrf_token(self, mock_set_score): + """ + Verify that put_result succeeds without a CSRF token even when the test + client has CSRF checks enabled. + + DRF's SessionAuthentication normally enforces CSRF on authenticated POSTs. + This test confirms that CsrfExemptSessionAuthentication bypasses that check, + so xqueue-watcher can post results using only a session cookie. + """ + csrf_client = APIClient(enforce_csrf_checks=True) + csrf_client.force_login(self.user) + + 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') + + self.assertEqual(response.status_code, status.HTTP_200_OK) From a5611e63e943304012a88505eb90799e02becee4 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Tue, 26 May 2026 12:25:00 -0400 Subject: [PATCH 3/8] fix: rename mock arg to _mock_set_score to satisfy pylint unused-argument --- submissions/tests/test_viewsets.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/submissions/tests/test_viewsets.py b/submissions/tests/test_viewsets.py index 0718fe1..ed54b7a 100644 --- a/submissions/tests/test_viewsets.py +++ b/submissions/tests/test_viewsets.py @@ -482,7 +482,7 @@ def test_get_submission_value_error(self, mock_get_files): self.assertIn("Unable to serialize submission payload", response.data['content']) @patch('submissions.api.set_score') - def test_put_result_succeeds_without_csrf_token(self, mock_set_score): + def test_put_result_succeeds_without_csrf_token(self, _mock_set_score): """ Verify that put_result succeeds without a CSRF token even when the test client has CSRF checks enabled. From 59586502e3f152ffddddfba087abe4d461c828d5 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Fri, 29 May 2026 15:24:55 -0400 Subject: [PATCH 4/8] fix: support GET on login endpoint to deliver CSRF cookie The root cause of the original 403 on put_result was that xqueue-watcher pre-fetches GET /xqueue/login/ to obtain a CSRF cookie before POSTing credentials, but the action only accepted POST, so the GET returned 405 and no cookie was set. Instead of bypassing CSRF enforcement entirely (CsrfExemptSessionAuthentication), accept GET on the login action and call get_token() to ensure the csrftoken cookie is present in the response. xqueue-watcher already reads that cookie and attaches its value as the X-CSRFToken header on every subsequent mutating request, so standard SessionAuthentication now works without any CSRF exemption. - Remove submissions/authentication.py (no longer needed) - Revert XQueueViewSet.authentication_classes to SessionAuthentication - Replace test_put_result_succeeds_without_csrf_token with: - test_login_get_sets_csrf_cookie: verifies the GET sets the cookie - test_xqueue_watcher_csrf_flow: end-to-end flow with enforce_csrf_checks=True confirming no bypass is required --- submissions/authentication.py | 24 ------------- submissions/tests/test_viewsets.py | 58 +++++++++++++++++++++++------- submissions/views/xqueue.py | 23 ++++++++++-- 3 files changed, 65 insertions(+), 40 deletions(-) delete mode 100644 submissions/authentication.py diff --git a/submissions/authentication.py b/submissions/authentication.py deleted file mode 100644 index d2bf815..0000000 --- a/submissions/authentication.py +++ /dev/null @@ -1,24 +0,0 @@ -"""Custom authentication classes for the submissions app.""" - -from rest_framework.authentication import SessionAuthentication - - -class CsrfExemptSessionAuthentication(SessionAuthentication): - """ - Session authentication that skips CSRF enforcement. - - The XQueue API is a service-to-service interface used by xqueue-watcher. - It is not a browser-facing endpoint, so CSRF protection is not applicable. - Authorization is handled instead by the IsXQueueUser permission class, which - requires the authenticated user to be a member of the 'xqueue' group. - - Security note: because this class disables CSRF while retaining cookie-based - session auth, it is important that the 'xqueue' group is restricted strictly - to dedicated service accounts and never granted to regular user accounts that - may also authenticate via a browser. Granting the 'xqueue' group to a - browser-accessible account would reintroduce a CSRF attack surface against - the state-changing endpoints (put_result, logout) of this API. - """ - - def enforce_csrf(self, request): - return # no-op: CSRF not required for service-to-service API calls diff --git a/submissions/tests/test_viewsets.py b/submissions/tests/test_viewsets.py index ed54b7a..13de818 100644 --- a/submissions/tests/test_viewsets.py +++ b/submissions/tests/test_viewsets.py @@ -16,7 +16,6 @@ from rest_framework.permissions import AllowAny from rest_framework.test import APIClient, APITestCase -from submissions.authentication import CsrfExemptSessionAuthentication from submissions.models import ExternalGraderDetail, SubmissionFile from submissions.permissions import IsXQueueUser from submissions.tests.factories import ExternalGraderDetailFactory, SubmissionFactory @@ -34,8 +33,6 @@ 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') @@ -62,7 +59,6 @@ def setUp(self): self.viewset = XQueueViewSet() def tearDown(self): - XQueueViewSet.authentication_classes = self._orig_auth_classes super().tearDown() def api_login(self): @@ -481,19 +477,50 @@ def test_get_submission_value_error(self, mock_get_files): self.assertEqual(response.data['return_code'], 1) self.assertIn("Unable to serialize submission payload", response.data['content']) - @patch('submissions.api.set_score') - def test_put_result_succeeds_without_csrf_token(self, _mock_set_score): + 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. """ - Verify that put_result succeeds without a CSRF token even when the test - client has CSRF checks enabled. + response = self.client.get(self.url_login) + self.assertEqual(response.status_code, status.HTTP_200_OK) + self.assertIn('csrftoken', response.cookies) - DRF's SessionAuthentication normally enforces CSRF on authenticated POSTs. - This test confirms that CsrfExemptSessionAuthentication bypasses that check, - so xqueue-watcher can post results using only a session cookie. + @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) - csrf_client.force_login(self.user) + # 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) + + # Step 3: POST put_result with session and CSRF token. self.external_grader.update_status('pulled') self.external_grader.refresh_from_db() @@ -505,6 +532,11 @@ def test_put_result_succeeds_without_csrf_token(self, _mock_set_score): 'xqueue_body': json.dumps({'score': 1}), } - response = csrf_client.post(self.url_put_result, payload, format='json') + 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 c3f685c..9241c8e 100644 --- a/submissions/views/xqueue.py +++ b/submissions/views/xqueue.py @@ -7,17 +7,18 @@ from django.contrib.auth import authenticate, login, logout from django.db import DatabaseError, transaction from django.db.models import Q +from django.middleware.csrf import get_token from django.http import HttpResponse from django.utils import timezone from openedx_events.learning.data import ExternalGraderScoreData from openedx_events.learning.signals import EXTERNAL_GRADER_SCORE_SUBMITTED from rest_framework import status, viewsets +from rest_framework.authentication import SessionAuthentication from rest_framework.decorators import action from rest_framework.permissions import AllowAny from rest_framework.response import Response from submissions.api import get_files_for_grader, set_score -from submissions.authentication import CsrfExemptSessionAuthentication from submissions.models import ExternalGraderDetail from submissions.permissions import IsXQueueUser @@ -59,7 +60,7 @@ class XQueueViewSet(viewsets.ViewSet): For more context, see DEPR: https://github.com/openedx/public-engineering/issues/286. """ - authentication_classes = [CsrfExemptSessionAuthentication] + authentication_classes = [SessionAuthentication] def get_permissions(self): """ @@ -73,12 +74,28 @@ def get_permissions(self): 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 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( From 24a334534f88784c23d11974d8410c20b2301ab7 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Fri, 29 May 2026 15:28:53 -0400 Subject: [PATCH 5/8] fix: remove no-op tearDown to satisfy pylint useless-parent-delegation --- submissions/tests/test_viewsets.py | 384 ++++++++++++++--------------- 1 file changed, 192 insertions(+), 192 deletions(-) diff --git a/submissions/tests/test_viewsets.py b/submissions/tests/test_viewsets.py index 13de818..0a71188 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 @@ -24,7 +25,7 @@ User = get_user_model() -@override_settings(ROOT_URLCONF='submissions.urls') +@override_settings(ROOT_URLCONF="submissions.urls") class TestXqueueViewSet(APITestCase): """ Test cases for XQueueViewSet endpoints. @@ -35,58 +36,52 @@ def setUp(self): super().setUp() # 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): - 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) @@ -98,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 = [] @@ -403,79 +393,87 @@ 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): """ @@ -488,9 +486,9 @@ def test_login_get_sets_csrf_cookie(self): """ response = self.client.get(self.url_login) self.assertEqual(response.status_code, status.HTTP_200_OK) - self.assertIn('csrftoken', response.cookies) + self.assertIn("csrftoken", response.cookies) - @patch('submissions.api.set_score') + @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 @@ -511,31 +509,33 @@ def test_xqueue_watcher_csrf_flow(self, _mock_set_score): # 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 + csrf_token = response.cookies["csrftoken"].value # Step 2: POST credentials to log in. response = csrf_client.post( self.url_login, - {'username': 'testuser', 'password': 'testpass'}, + {"username": "testuser", "password": "testpass"}, ) self.assertEqual(response.status_code, status.HTTP_200_OK) # Step 3: POST put_result with session and CSRF token. - self.external_grader.update_status('pulled') + 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}), + "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', + format="json", HTTP_X_CSRFTOKEN=csrf_token, ) From 2e66192d25d898f09222275a14b34cf345a70dd0 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Thu, 4 Jun 2026 08:29:53 -0400 Subject: [PATCH 6/8] fix: address make test and make test_quality failures - Makefile: prefix pylint with DJANGO_SETTINGS_MODULE=settings so that the pylint_django plugin does not emit E5110 (django-not-configured) - settings.py: add CsrfViewMiddleware to MIDDLEWARE; without it, get_token(request) sets CSRF_COOKIE_NEEDS_UPDATE but nothing processes that flag into a Set-Cookie header, so the CSRF cookie was never sent - submissions/tests/test_viewsets.py: after the login POST in test_xqueue_watcher_csrf_flow, re-read csrf_token from the response cookies; django.contrib.auth.login() calls rotate_token() which issues a new CSRF secret, making the token captured from the preceding GET stale - submissions/views/xqueue.py: move django.http import before django.middleware.csrf to satisfy isort; also apply consistent double-quote style throughout the file --- Makefile | 2 +- settings.py | 81 +++++----- submissions/tests/test_viewsets.py | 4 + submissions/views/xqueue.py | 233 +++++++++++++++-------------- 4 files changed, 164 insertions(+), 156 deletions(-) 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/tests/test_viewsets.py b/submissions/tests/test_viewsets.py index 0a71188..72f2790 100644 --- a/submissions/tests/test_viewsets.py +++ b/submissions/tests/test_viewsets.py @@ -517,6 +517,10 @@ def test_xqueue_watcher_csrf_flow(self, _mock_set_score): {"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") diff --git a/submissions/views/xqueue.py b/submissions/views/xqueue.py index 9241c8e..0041baa 100644 --- a/submissions/views/xqueue.py +++ b/submissions/views/xqueue.py @@ -7,8 +7,8 @@ from django.contrib.auth import authenticate, login, logout from django.db import DatabaseError, transaction from django.db.models import Q -from django.middleware.csrf import get_token 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 @@ -68,13 +68,13 @@ 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=['get', 'post'], url_name='login') + @action(detail=False, methods=["get", "post"], url_name="login") def login(self, request): """ Endpoint for authenticating users and creating sessions. @@ -85,56 +85,49 @@ def login(self, request): POST: Authenticates the supplied username/password and opens a session. """ - if request.method == 'GET': + 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 - ) + 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') + 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): """ @@ -145,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: @@ -180,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: @@ -260,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: @@ -281,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( @@ -322,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): """ @@ -359,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 @@ -379,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) @@ -406,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")) From 9aee34e7d967e3299a5b2dbca1813d76a75b8468 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Thu, 18 Jun 2026 19:53:34 -0400 Subject: [PATCH 7/8] chore: bump version to 4.0.1 to force reinstall from git override --- submissions/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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' From 8660d2c95975af53546ab86ee27dde429b64a869 Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Mon, 22 Jun 2026 13:07:01 -0400 Subject: [PATCH 8/8] fix: update codecov-action to v7.0.0 and add fail-fast: false codecov-action v6.0.1 cannot verify the GPG signature of the codecov CLI binary (key 27034E7F, added April 21 2026). v7.0.0 (June 7 2026) ships with the updated key. fail-fast: false prevents a single matrix job failure from cancelling the remaining jobs. Co-Authored-By: Claude Sonnet 4.6 Claude-Session: https://claude.ai/code/session_016ZeSUL6HTwgMGN4JqPzf2S --- .github/workflows/ci.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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