diff --git a/CHANGELOG.rst b/CHANGELOG.rst index ffe09487c6..ef331f4388 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,21 @@ Change Log Unreleased ~~~~~~~~~~ +[6.1.0] - 2026-07-28 + +* Make proctored exam attempt removal fail loudly when the proctoring provider errors, + instead of a silent local-only reset or an unhandled 500: + + * Add an explicit request timeout to the REST proctoring backend (default 30s, + overridable per backend via a ``timeout`` key in ``PROCTORING_BACKENDS``). + * Raise a typed ``BackendProviderCannotRemoveAttempt`` (HTTP 502) both when the + provider is unreachable/times out and when it responds with an HTTP error status, + returning a clear message (including the provider's HTTP status) and logging the raw + provider response for debugging. Callers can then return a descriptive error to the + instructor instead of a silent local-only reset or an unhandled 500. Raising on a + provider HTTP error is on by default and overridable per backend via a + ``raise_on_remove_error`` key in ``PROCTORING_BACKENDS``. + [5.2.1] - 2025-12-05 * Remove all references to Proctortrack diff --git a/edx_proctoring/__init__.py b/edx_proctoring/__init__.py index be497164f0..7b715fdba2 100644 --- a/edx_proctoring/__init__.py +++ b/edx_proctoring/__init__.py @@ -3,4 +3,4 @@ """ # Be sure to update the version number in edx_proctoring/package.json -__version__ = '6.0.0' +__version__ = '6.1.0' diff --git a/edx_proctoring/apps.py b/edx_proctoring/apps.py index eba34af307..145d66e685 100644 --- a/edx_proctoring/apps.py +++ b/edx_proctoring/apps.py @@ -30,6 +30,7 @@ 'organization', 'passing_statuses', 'ping_interval', + 'raise_on_remove_error', 'secret_key', 'secret_key_id', 'send_email', @@ -37,6 +38,7 @@ 'supports_onboarding', 'tech_support_email', 'tech_support_phone', + 'timeout', 'token_expiration_time', 'verbose_name', 'video_review_aes_key', diff --git a/edx_proctoring/backends/rest.py b/edx_proctoring/backends/rest.py index f10e756ab6..b3886c1321 100644 --- a/edx_proctoring/backends/rest.py +++ b/edx_proctoring/backends/rest.py @@ -19,6 +19,7 @@ from edx_proctoring.backends.backend import ProctoringBackendProvider from edx_proctoring.exceptions import ( BackendProviderCannotRegisterAttempt, + BackendProviderCannotRemoveAttempt, BackendProviderCannotRetireUser, BackendProviderOnboardingException, BackendProviderOnboardingProfilesException, @@ -41,6 +42,15 @@ class BaseRestProctoringProvider(ProctoringBackendProvider): has_dashboard = True supports_onboarding = True passing_statuses = (SoftwareSecureReviewStatus.clean,) + # Timeout (in seconds) applied to every outbound request to the provider so + # that a slow or unavailable provider cannot hang the request indefinitely. + # Operators can override this per backend via a ``timeout`` key in the + # backend's ``PROCTORING_BACKENDS`` configuration. + timeout = 30 + # Whether a provider HTTP error while removing an attempt should raise a descriptive + # error (so it is surfaced to the instructor) rather than being logged and ignored. + # Overridable per backend via a ``raise_on_remove_error`` key in ``PROCTORING_BACKENDS``. + raise_on_remove_error = True @property def exam_attempt_url(self): @@ -227,12 +237,18 @@ def stop_exam_attempt(self, exam, attempt): def remove_exam_attempt(self, exam, attempt): """ - Removes the exam attempt on the backend provider's server + Removes the exam attempt on the backend provider's server. + + Raises BackendProviderCannotRemoveAttempt if the provider responds with an + HTTP error status (unless ``raise_on_remove_error`` is disabled for this backend), + so the caller can surface a descriptive error instead of silently leaving the + attempt on the provider's side. """ response = self._make_attempt_request( exam, attempt, - method='DELETE') + method='DELETE', + raise_on_error=self.raise_on_remove_error) return response.get('status', None) == 'deleted' def mark_erroneous_exam_attempt(self, exam, attempt): @@ -369,9 +385,14 @@ def _get_language_headers(self): lang_header = f'{current_lang};{default_lang}' return {'Accept-Language': lang_header} - def _make_attempt_request(self, exam, attempt, method='POST', status=None, **payload): + def _make_attempt_request(self, exam, attempt, method='POST', status=None, raise_on_error=False, **payload): """ - Calls backend attempt API + Calls backend attempt API. + + When ``raise_on_error`` is True, a provider HTTP error response (status >= 400) + raises BackendProviderCannotRemoveAttempt, surfacing the provider's own message + when it supplies one and a generic message otherwise. This is distinct from the + provider being unreachable/timing out, where ``session.request`` itself raises. """ if not attempt: return {} @@ -384,10 +405,23 @@ def _make_attempt_request(self, exam, attempt, method='POST', status=None, **pay if method == 'GET': headers.update(self._get_language_headers()) log.debug('Making %r attempt request at %r', method, url) - response = self.session.request(method, url, json=payload, headers=headers) + response = self.session.request(method, url, json=payload, headers=headers, timeout=self.timeout) try: data = response.json() except ValueError: log.exception("Decoding attempt %r -> %r", attempt, response.content) data = {} + if raise_on_error and not response.ok: + # Log the raw provider response (status + body) for support/debugging; the + # provider's error schema is not standardised, so we do not try to parse a + # message out of it and instead surface a clean, provider-status-aware error. + log.error( + 'Proctoring provider returned HTTP %s while removing attempt %r: %r', + response.status_code, attempt, response.content, + ) + raise BackendProviderCannotRemoveAttempt( + 'The proctoring provider could not remove this attempt ' + f'(provider returned HTTP {response.status_code}). ' + 'Please try again or contact support.' + ) return data diff --git a/edx_proctoring/backends/tests/test_rest.py b/edx_proctoring/backends/tests/test_rest.py index f328beb71e..1dc1091efe 100644 --- a/edx_proctoring/backends/tests/test_rest.py +++ b/edx_proctoring/backends/tests/test_rest.py @@ -12,9 +12,11 @@ from django.test import TestCase, override_settings from django.utils import translation +from edx_proctoring.apps import BACKEND_CONFIGURATION_ALLOW_LIST from edx_proctoring.backends.rest import BaseRestProctoringProvider from edx_proctoring.exceptions import ( BackendProviderCannotRegisterAttempt, + BackendProviderCannotRemoveAttempt, BackendProviderCannotRetireUser, BackendProviderOnboardingException, BackendProviderOnboardingProfilesException, @@ -277,6 +279,104 @@ def test_remove_attempt_no_attempt(self): status = self.provider.remove_exam_attempt(self.backend_exam['external_id'], None) self.assertFalse(status) + def test_make_attempt_request_passes_timeout(self): + """ + Every outbound attempt request must include an explicit timeout so that a + slow/unavailable provider cannot hang the request indefinitely. + """ + attempt_id = 2 + with patch.object(self.provider.session, 'request') as request_mock: + request_mock.return_value.json.return_value = {'status': 'deleted'} + self.provider.remove_exam_attempt(self.backend_exam['external_id'], attempt_id) + _, kwargs = request_mock.call_args + self.assertEqual(kwargs['timeout'], self.provider.timeout) + + def test_default_timeout(self): + """The backend applies a sane default request timeout when none is configured.""" + self.assertEqual(self.provider.timeout, 30) + + def test_timeout_is_configurable(self): + """ + Operators can override the request timeout per backend. The value arrives as a + constructor kwarg sourced from the backend's PROCTORING_BACKENDS configuration, + so the key must also be present in the backend configuration allow list. + """ + provider = BaseRestProctoringProvider('client_id', 'client_secret', timeout=60) + self.assertEqual(provider.timeout, 60) + self.assertIn('timeout', BACKEND_CONFIGURATION_ALLOW_LIST) + + def test_remove_attempt_provider_unavailable(self): + """ + A provider connection error must propagate out of the backend rather than + being swallowed, so the caller can surface a descriptive error. + """ + attempt_id = 2 + with patch.object(self.provider.session, 'request', side_effect=ConnectionError('boom')): + with self.assertRaises(ConnectionError): + self.provider.remove_exam_attempt(self.backend_exam['external_id'], attempt_id) + + @responses.activate + def test_remove_attempt_provider_error_raises_with_status(self): + """ + A provider HTTP error response (>= 400) raises BackendProviderCannotRemoveAttempt + with a clean, provider-status-aware message and an instructor-facing 502. + """ + attempt_id = 2 + responses.add( + responses.DELETE, + url=self.provider.exam_attempt_url.format(exam_id=self.backend_exam['external_id'], attempt_id=attempt_id), + json={'detail': 'Attempt is locked'}, + status=400, + ) + with self.assertRaises(BackendProviderCannotRemoveAttempt) as ctx: + self.provider.remove_exam_attempt(self.backend_exam['external_id'], attempt_id) + self.assertIn('HTTP 400', str(ctx.exception)) + self.assertIn('could not remove this attempt', str(ctx.exception)) + self.assertEqual(ctx.exception.http_status, 502) + + @responses.activate + def test_remove_attempt_provider_error_no_body(self): + """ + A provider HTTP error with no response body still raises with the clean + provider-status-aware message. + """ + attempt_id = 2 + responses.add( + responses.DELETE, + url=self.provider.exam_attempt_url.format(exam_id=self.backend_exam['external_id'], attempt_id=attempt_id), + status=502, + ) + with self.assertRaises(BackendProviderCannotRemoveAttempt) as ctx: + self.provider.remove_exam_attempt(self.backend_exam['external_id'], attempt_id) + self.assertIn('HTTP 502', str(ctx.exception)) + self.assertEqual(ctx.exception.http_status, 502) + + def test_raise_on_remove_error_default_and_configurable(self): + """ + The raise-on-remove-error behaviour defaults to True and is overridable per + backend via PROCTORING_BACKENDS, so the key must be in the allow list. + """ + self.assertTrue(self.provider.raise_on_remove_error) + self.assertIn('raise_on_remove_error', BACKEND_CONFIGURATION_ALLOW_LIST) + provider = BaseRestProctoringProvider('client_id', 'client_secret', raise_on_remove_error=False) + self.assertFalse(provider.raise_on_remove_error) + + @responses.activate + def test_remove_attempt_provider_error_not_raised_when_disabled(self): + """ + When raise_on_remove_error is disabled for a backend, a provider HTTP error is + not raised; removal returns False (the pre-existing log-and-continue behaviour). + """ + self.provider.raise_on_remove_error = False + attempt_id = 2 + responses.add( + responses.DELETE, + url=self.provider.exam_attempt_url.format(exam_id=self.backend_exam['external_id'], attempt_id=attempt_id), + status=502, + ) + status = self.provider.remove_exam_attempt(self.backend_exam['external_id'], attempt_id) + self.assertFalse(status) + def test_on_review_callback(self): """ on_review_callback should just return the payload diff --git a/edx_proctoring/exceptions.py b/edx_proctoring/exceptions.py index 26581c65ac..feec824c16 100644 --- a/edx_proctoring/exceptions.py +++ b/edx_proctoring/exceptions.py @@ -95,6 +95,15 @@ class BackendProviderSentNoAttemptID(BackendProviderCannotRegisterAttempt): """ +class BackendProviderCannotRemoveAttempt(ProctoredBaseException): + """ + Raised when a back-end provider cannot remove an attempt, e.g. because the + provider is slow, unreachable, or returned an error while trying to delete + the attempt on their side. + """ + http_status = status.HTTP_502_BAD_GATEWAY + + class BackendProviderOnboardingException(ProctoredBaseException): """ Raised when a back-end provider cannot register an attempt diff --git a/edx_proctoring/handlers.py b/edx_proctoring/handlers.py index 44fecc7c9a..ff13631452 100644 --- a/edx_proctoring/handlers.py +++ b/edx_proctoring/handlers.py @@ -7,6 +7,7 @@ from edx_proctoring import api, constants, models from edx_proctoring.backends import get_backend_provider +from edx_proctoring.exceptions import BackendProviderCannotRemoveAttempt from edx_proctoring.runtime import get_runtime_service from edx_proctoring.statuses import ProctoredExamStudentAttemptStatus, SoftwareSecureReviewStatus from edx_proctoring.utils import emit_event, locate_attempt_by_attempt_code @@ -138,7 +139,40 @@ def on_attempt_changed(sender, instance, signal, **kwargs): if instance.proctored_exam.is_proctored: backend = get_backend_provider(name=instance.proctored_exam.backend) if backend: - result = backend.remove_exam_attempt(instance.proctored_exam.external_id, instance.external_id) + log_context = { + 'attempt_id': instance.id, + 'external_id': instance.external_id, + 'exam_id': instance.proctored_exam.id, + 'user_id': instance.user_id, + 'backend': instance.proctored_exam.backend, + } + log_message = ( + 'Failed to remove attempt_id=%(attempt_id)s (external_id=%(external_id)s) for ' + 'exam_id=%(exam_id)s user_id=%(user_id)s from backend=%(backend)s due to a ' + 'provider error.' + ) + try: + result = backend.remove_exam_attempt( + instance.proctored_exam.external_id, instance.external_id + ) + except BackendProviderCannotRemoveAttempt: + # The provider responded with an HTTP error and the backend already built a + # descriptive (possibly provider-supplied) message. Log and re-raise it as-is + # so that message reaches the instructor-facing MFE unchanged. + log.exception(log_message, log_context) + raise + except Exception as exc: # pylint: disable=broad-exception-caught + # The provider is slow/unavailable (connection error or timeout). This must not + # surface as an unhandled 500 or silently leave the attempt half-removed. Log + # with enough context for monitoring/Sentry and re-raise a typed exception so the + # instructor-facing MFE receives a descriptive ``detail`` message. + log.exception(log_message, log_context) + raise BackendProviderCannotRemoveAttempt( + # Translators: shown to an instructor when an exam attempt could not be reset + # because the external proctoring provider is temporarily unavailable. + 'The proctoring provider is temporarily unavailable, so this attempt could not ' + 'be fully reset. Please try again in a few minutes.' + ) from exc if not result: log.error( 'Failed to remove attempt_id=%s from backend=%s', diff --git a/edx_proctoring/tests/test_handlers.py b/edx_proctoring/tests/test_handlers.py index 1dcd4e6b2c..43c0a0b0c3 100644 --- a/edx_proctoring/tests/test_handlers.py +++ b/edx_proctoring/tests/test_handlers.py @@ -6,9 +6,11 @@ import ddt from httmock import HTTMock +from django.db import transaction from django.db.models.signals import pre_delete, pre_save from edx_proctoring.api import update_attempt_status +from edx_proctoring.exceptions import BackendProviderCannotRemoveAttempt from edx_proctoring.models import ProctoredExam, ProctoredExamStudentAttempt from edx_proctoring.statuses import ProctoredExamStudentAttemptStatus from edx_proctoring.tests.test_services import MockInstructorService @@ -47,6 +49,54 @@ def test_backend_fails_to_delete_attempt(self, logger_mock): log_format_string = 'Failed to remove attempt_id=%s from backend=%s' logger_mock.assert_any_call(log_format_string, 1, self.backend_name) + @patch('logging.Logger.exception') + @patch('edx_proctoring.handlers.get_backend_provider') + def test_provider_unavailable_raises_and_keeps_attempt(self, get_backend_mock, logger_mock): + """ + If the provider raises (slow/unavailable) while removing the attempt, a typed + BackendProviderCannotRemoveAttempt must be raised, the failure logged with + context, and the local attempt must NOT be deleted. + """ + attempt_id = self.attempt.id + backend = get_backend_mock.return_value + backend.remove_exam_attempt.side_effect = ConnectionError('provider down') + + # ``delete()`` opens an inner atomic(savepoint=False); when the pre_delete + # handler raises, that marks the connection as needing rollback. Wrapping the + # call in an explicit atomic block creates a savepoint that absorbs the + # rollback, so the connection is usable again for the assertion below. + with self.assertRaises(BackendProviderCannotRemoveAttempt) as context: + with transaction.atomic(): + self.attempt.delete_exam_attempt() + + self.assertIn('temporarily unavailable', str(context.exception)) + self.assertEqual(context.exception.http_status, 502) + logger_mock.assert_called_once() + # the attempt must still exist because the removal failed + self.assertTrue(ProctoredExamStudentAttempt.objects.filter(id=attempt_id).exists()) + + @patch('logging.Logger.exception') + @patch('edx_proctoring.handlers.get_backend_provider') + def test_provider_error_response_preserves_message_and_keeps_attempt(self, get_backend_mock, logger_mock): + """ + If the backend raises BackendProviderCannotRemoveAttempt (the provider responded + with an HTTP error), the handler re-raises it unchanged so the provider-supplied + message reaches the caller, and the local attempt must NOT be deleted. + """ + attempt_id = self.attempt.id + backend = get_backend_mock.return_value + backend.remove_exam_attempt.side_effect = BackendProviderCannotRemoveAttempt('Attempt is locked on provider') + + with self.assertRaises(BackendProviderCannotRemoveAttempt) as context: + with transaction.atomic(): + self.attempt.delete_exam_attempt() + + # the provider's message is preserved (not overwritten by the generic one) + self.assertIn('Attempt is locked on provider', str(context.exception)) + self.assertEqual(context.exception.http_status, 502) + logger_mock.assert_called_once() + self.assertTrue(ProctoredExamStudentAttempt.objects.filter(id=attempt_id).exists()) + @ddt.data(None, MockInstructorService()) @patch('edx_proctoring.handlers.get_runtime_service') @patch('edx_proctoring.tests.test_services.MockInstructorService.complete_student_attempt') diff --git a/package.json b/package.json index c6cc63a68b..d8f9329042 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "@edx/edx-proctoring", "//": "Note that the version format is slightly different than that of the Python version when using prereleases.", - "version": "6.0.0", + "version": "6.1.0", "main": "edx_proctoring/static/index.js", "scripts": { "test": "gulp test"