diff --git a/cms/djangoapps/contentstore/rest_api/v1/tests/__init__.py b/cms/djangoapps/contentstore/rest_api/v1/tests/__init__.py new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/cms/djangoapps/contentstore/rest_api/v1/tests/test_course_details.py b/cms/djangoapps/contentstore/rest_api/v1/tests/test_course_details.py new file mode 100644 index 000000000000..576a43b3ce9f --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/tests/test_course_details.py @@ -0,0 +1,100 @@ +""" +ADR 0029 – Standardized error-response tests for CourseDetailsViewSet. + +Tests that auth/permission/not-found error responses conform to the ADR 0029 +JSON envelope after removing DeveloperErrorViewMixin and @verify_course_exists(). +""" +from unittest.mock import patch + +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APIClient, APITestCase + +from common.djangoapps.student.tests.factories import UserFactory + +# A syntactically valid course key that does not exist in the DB. +TEST_COURSE_ID = "course-v1:TestOrg+TestCourse+2026" +MOCK_COURSE_EXISTS = ( + "cms.djangoapps.contentstore.rest_api.v1.views.course_details.CourseOverview.course_exists" +) + +_REQUIRED_ERROR_FIELDS = ("type", "title", "status", "detail", "instance") + + +class TestCourseDetailsViewSetErrorShape(APITestCase): + """ + ADR 0029 – error response shape regression tests for CourseDetailsViewSet. + + Verifies that 401, 403, and 404 responses use the standardized envelope + after removing DeveloperErrorViewMixin and @verify_course_exists(). + """ + + def setUp(self): + super().setUp() + self.client = APIClient() + self.detail_url = reverse( + "cms.djangoapps.contentstore:v1:course_details-detail", + kwargs={"course_id": TEST_COURSE_ID}, + ) + + def test_unauthenticated_get_returns_standardized_401(self): + """Unauthenticated GET must return 401 with the ADR 0029 envelope.""" + response = self.client.get(self.detail_url) + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) + for field in _REQUIRED_ERROR_FIELDS: + self.assertIn(field, response.data, f"ADR 0029: missing field '{field}'") + + def test_unauthenticated_401_type_uri(self): + """The ``type`` field for 401 must be the ADR 0029 authn URI.""" + response = self.client.get(self.detail_url) + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) + self.assertEqual(response.data.get("type"), "https://docs.openedx.org/errors/authn") + + def test_non_author_get_returns_standardized_403(self): + """Authenticated non-author GET must return 403 with the ADR 0029 envelope.""" + non_author = UserFactory.create() + self.client.force_authenticate(user=non_author) + response = self.client.get(self.detail_url) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + for field in _REQUIRED_ERROR_FIELDS: + self.assertIn(field, response.data, f"ADR 0029: missing field '{field}'") + + def test_non_author_403_type_uri(self): + """The ``type`` field for 403 must be the ADR 0029 authz URI.""" + non_author = UserFactory.create() + self.client.force_authenticate(user=non_author) + response = self.client.get(self.detail_url) + self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN) + self.assertEqual(response.data.get("type"), "https://docs.openedx.org/errors/authz") + + @patch(MOCK_COURSE_EXISTS, return_value=False) + def test_nonexistent_course_returns_standardized_404(self, _mock): + """GET for a non-existent course must return 404 with the ADR 0029 envelope.""" + staff = UserFactory.create(is_staff=True) + self.client.force_authenticate(user=staff) + response = self.client.get(self.detail_url) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + for field in _REQUIRED_ERROR_FIELDS: + self.assertIn(field, response.data, f"ADR 0029: missing field '{field}'") + + @patch(MOCK_COURSE_EXISTS, return_value=False) + def test_not_found_type_uri(self, _mock): + """The ``type`` field for 404 must be the ADR 0029 not-found URI.""" + staff = UserFactory.create(is_staff=True) + self.client.force_authenticate(user=staff) + response = self.client.get(self.detail_url) + self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) + self.assertEqual(response.data.get("type"), "https://docs.openedx.org/errors/not-found") + + def test_error_body_has_no_developer_message(self): + """Error responses must NOT contain the old DeveloperErrorViewMixin fields.""" + response = self.client.get(self.detail_url) + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) + self.assertNotIn("developer_message", response.data) + self.assertNotIn("error_code", response.data) + + def test_instance_field_is_request_path(self): + """The ``instance`` field must equal the request path.""" + response = self.client.get(self.detail_url) + self.assertEqual(response.status_code, status.HTTP_401_UNAUTHORIZED) + self.assertEqual(response.data.get("instance"), self.detail_url) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/course_details.py b/cms/djangoapps/contentstore/rest_api/v1/views/course_details.py index 4d2a8491f8d7..ee660488b985 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/course_details.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/course_details.py @@ -1,9 +1,11 @@ """ API Views for course details """ import edx_api_doc_tools as apidocs -from django.core.exceptions import ValidationError -from common.djangoapps.util.json_request import JsonResponseBadRequest +from django.core.exceptions import ValidationError as DjangoValidationError +from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey +from rest_framework.exceptions import NotFound +from rest_framework.exceptions import ValidationError as DRFValidationError from rest_framework.permissions import IsAuthenticated from rest_framework.request import Request from rest_framework.response import Response @@ -11,8 +13,8 @@ from rest_framework.views import APIView from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.models.course_details import CourseDetails -from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, verify_course_exists from xmodule.modulestore.django import modulestore from cms.djangoapps.contentstore.views.permissions import HasStudioReadAccess @@ -21,7 +23,7 @@ # ADR 0028 – consolidated from CourseDetailsView -class CourseDetailsViewSet(DeveloperErrorViewMixin, viewsets.ViewSet): +class CourseDetailsViewSet(viewsets.ViewSet): """ ViewSet for course details. Registered via DefaultRouter (basename ``course_details``). @@ -31,8 +33,7 @@ class CourseDetailsViewSet(DeveloperErrorViewMixin, viewsets.ViewSet): ADR 0025 compliance notes: - ``serializer_class`` declared; used for both response serialization and apidocs schema. - - Request validation is handled by ``update_course_details()`` (service layer) and - the ``@verify_course_exists()`` decorator. + - Request validation is handled by ``update_course_details()`` (service layer). """ authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) @@ -54,7 +55,6 @@ class CourseDetailsViewSet(DeveloperErrorViewMixin, viewsets.ViewSet): 404: "The requested course does not exist.", }, ) - @verify_course_exists() def retrieve(self, request: Request, course_id: str): """ Get an object containing all the course details. @@ -70,7 +70,12 @@ def retrieve(self, request: Request, course_id: str): The HTTP 200 response contains a single dict that contains keys that are the course's details. """ - course_key = CourseKey.from_string(course_id) + try: + course_key = CourseKey.from_string(course_id) + except InvalidKeyError: + raise NotFound("The provided course key cannot be parsed.") # noqa: B904 + if not CourseOverview.course_exists(course_key): + raise NotFound(f"Course {course_id} not found.") course_details = CourseDetails.fetch(course_key) serializer = self.serializer_class(course_details) return Response(serializer.data) @@ -87,7 +92,6 @@ def retrieve(self, request: Request, course_id: str): 404: "The requested course does not exist.", }, ) - @verify_course_exists() def update(self, request: Request, course_id: str): """ Update a course's details. @@ -108,13 +112,18 @@ def update(self, request: Request, course_id: str): If the request is successful, an HTTP 200 "OK" response is returned, along with all the course's details similar to a ``GET`` request. """ - course_key = CourseKey.from_string(course_id) + try: + course_key = CourseKey.from_string(course_id) + except InvalidKeyError: + raise NotFound("The provided course key cannot be parsed.") # noqa: B904 + if not CourseOverview.course_exists(course_key): + raise NotFound(f"Course {course_id} not found.") course_block = modulestore().get_course(course_key) try: updated_data = update_course_details(request, course_key, request.data, course_block) - except ValidationError as err: - return JsonResponseBadRequest({"error": err.message}) + except DjangoValidationError as err: + raise DRFValidationError(err.message) from err serializer = self.serializer_class(updated_data) return Response(serializer.data) @@ -122,13 +131,14 @@ def update(self, request: Request, course_id: str): # DEPRECATED (ADR 0028): Use CourseDetailsViewSet instead. # Will be removed after one named release. Use GET/PUT course_details/{course_id}/ instead. -class CourseDetailsView(DeveloperErrorViewMixin, APIView): +class CourseDetailsView(APIView): """ View for getting and setting the course details. """ authentication_classes = (JwtAuthentication, SessionAuthenticationAllowInactiveUser) permission_classes = (IsAuthenticated, HasStudioReadAccess) serializer_class = CourseDetailsSerializer + @apidocs.schema( parameters=[ apidocs.string_parameter("course_id", apidocs.ParameterLocation.PATH, description="Course ID"), @@ -140,7 +150,6 @@ class CourseDetailsView(DeveloperErrorViewMixin, APIView): 404: "The requested course does not exist.", }, ) - @verify_course_exists() def get(self, request: Request, course_id: str): """ Get an object containing all the course details. @@ -205,7 +214,12 @@ def get(self, request: Request, course_id: str): } ``` """ - course_key = CourseKey.from_string(course_id) + try: + course_key = CourseKey.from_string(course_id) + except InvalidKeyError: + raise NotFound("The provided course key cannot be parsed.") # noqa: B904 + if not CourseOverview.course_exists(course_key): + raise NotFound(f"Course {course_id} not found.") course_details = CourseDetails.fetch(course_key) serializer = self.serializer_class(course_details) return Response(serializer.data) @@ -222,7 +236,6 @@ def get(self, request: Request, course_id: str): 404: "The requested course does not exist.", }, ) - @verify_course_exists() def put(self, request: Request, course_id: str): """ Update a course's details. @@ -245,13 +258,18 @@ def put(self, request: Request, course_id: str): If the request is successful, an HTTP 200 "OK" response is returned, along with all the course's details similar to a ``GET`` request. """ - course_key = CourseKey.from_string(course_id) + try: + course_key = CourseKey.from_string(course_id) + except InvalidKeyError: + raise NotFound("The provided course key cannot be parsed.") # noqa: B904 + if not CourseOverview.course_exists(course_key): + raise NotFound(f"Course {course_id} not found.") course_block = modulestore().get_course(course_key) try: updated_data = update_course_details(request, course_key, request.data, course_block) - except ValidationError as err: - return JsonResponseBadRequest({"error": err.message}) + except DjangoValidationError as err: + raise DRFValidationError(err.message) from err serializer = self.serializer_class(updated_data) return Response(serializer.data) diff --git a/openedx/core/lib/api/exceptions.py b/openedx/core/lib/api/exceptions.py new file mode 100644 index 000000000000..ac4b1b6ede22 --- /dev/null +++ b/openedx/core/lib/api/exceptions.py @@ -0,0 +1,148 @@ +""" +ADR 0029 – Standardized error-response exception handler and helpers. + +Installs a platform-level DRF ``EXCEPTION_HANDLER`` that converts every +API exception into a single, consistent JSON envelope:: + + { + "type": "https://docs.openedx.org/errors/", + "title": "Validation Error", + "status": 400, + "detail": "The request body failed validation.", + "instance": "/api/enrollment/v1/enrollment/", + "user_message": "...", # optional – present only when set on exc + "errors": {...} # optional – present only for ValidationError + } + +The handler chains through the existing ``ignored_error_exception_handler`` +so that error logging / monitoring added by that handler is preserved. +""" + +from rest_framework.exceptions import APIException, ValidationError +from rest_framework.response import Response + + +# --------------------------------------------------------------------------- +# Public exception classes +# --------------------------------------------------------------------------- + +class Conflict(APIException): + """HTTP 409 Conflict — ADR 0029.""" + + status_code = 409 + default_detail = "A conflict occurred." + default_code = "conflict" + + +# --------------------------------------------------------------------------- +# Central handler +# --------------------------------------------------------------------------- + +def standardized_error_exception_handler(exc, context): + """ + ADR 0029 – platform-level DRF exception handler. + + Chains through ``ignored_error_exception_handler`` so that its error + logging / monitoring is preserved, then reformats the response to the + ADR 0029 envelope shape. + + Returns a generic 500 body for unhandled exceptions so that stack + traces are never leaked to callers. + """ + from openedx.core.lib.request_utils import ignored_error_exception_handler + response = ignored_error_exception_handler(exc, context) + + if response is None: + return Response( + { + "type": "https://docs.openedx.org/errors/internal", + "title": "Internal Server Error", + "status": 500, + "detail": "An unexpected error occurred. Please try again later.", + }, + status=500, + ) + + request = context.get("request") + body = { + "type": f"https://docs.openedx.org/errors/{_error_type(exc)}", + "title": _error_title(exc), + "status": response.status_code, + "detail": _flatten_detail(response.data), + } + if request: + body["instance"] = request.path + if hasattr(exc, "user_message") and exc.user_message: + body["user_message"] = exc.user_message + if isinstance(exc, ValidationError) and hasattr(exc, "detail"): + body["errors"] = _normalize_validation_errors(exc.detail) + + response.data = body + response["Content-Type"] = "application/json" + return response + + +# --------------------------------------------------------------------------- +# Private helpers +# --------------------------------------------------------------------------- + +def _error_type(exc): + """Map a DRF exception to an ADR 0029 error-type slug.""" + from rest_framework.exceptions import ( + AuthenticationFailed, NotAuthenticated, NotFound, PermissionDenied, + Throttled, ValidationError, + ) + if isinstance(exc, (NotAuthenticated, AuthenticationFailed)): + return "authn" + if isinstance(exc, PermissionDenied): + return "authz" + if isinstance(exc, NotFound): + return "not-found" + if isinstance(exc, ValidationError): + return "validation" + if isinstance(exc, Throttled): + return "rate-limited" + if isinstance(exc, Conflict): + return "conflict" + return "internal" + + +def _error_title(exc): + """Return a short, developer-facing title for the exception class.""" + from rest_framework.exceptions import ( + AuthenticationFailed, NotAuthenticated, NotFound, PermissionDenied, + Throttled, ValidationError, + ) + _TITLES = { + NotAuthenticated: "Authentication Required", + AuthenticationFailed: "Authentication Failed", + PermissionDenied: "Permission Denied", + NotFound: "Not Found", + ValidationError: "Validation Error", + Throttled: "Too Many Requests", + Conflict: "Conflict", + } + return _TITLES.get(type(exc), "Internal Server Error") + + +def _flatten_detail(data): + """Extract a single string from DRF's response.data for the ``detail`` field.""" + if isinstance(data, str): + return data + if isinstance(data, dict) and "detail" in data: + return str(data["detail"]) + if isinstance(data, list) and data: + return str(data[0]) + return str(data) + + +def _normalize_validation_errors(detail): + """Normalize DRF ValidationError detail into ``{field: [msg, ...]}`` form.""" + if isinstance(detail, dict): + return { + field: [str(e) for e in (errs if isinstance(errs, list) else [errs])] + for field, errs in detail.items() + } + if isinstance(detail, list): + return {"non_field_errors": [str(e) for e in detail]} + return {"non_field_errors": [str(detail)]} diff --git a/openedx/envs/common.py b/openedx/envs/common.py index 5d7c105025ed..1247116e397c 100644 --- a/openedx/envs/common.py +++ b/openedx/envs/common.py @@ -820,7 +820,7 @@ def add_optional_apps(optional_apps, installed_apps): 'DEFAULT_RENDERER_CLASSES': ( 'rest_framework.renderers.JSONRenderer', ), - 'EXCEPTION_HANDLER': 'openedx.core.lib.request_utils.ignored_error_exception_handler', + 'EXCEPTION_HANDLER': 'openedx.core.lib.api.exceptions.standardized_error_exception_handler', # ADR 0029 'PAGE_SIZE': 10, 'URL_FORMAT_OVERRIDE': None, 'DEFAULT_THROTTLE_RATES': {