diff --git a/cms/djangoapps/contentstore/rest_api/v0/views/xblock.py b/cms/djangoapps/contentstore/rest_api/v0/views/xblock.py index 79217971bb67..1cab4a390570 100644 --- a/cms/djangoapps/contentstore/rest_api/v0/views/xblock.py +++ b/cms/djangoapps/contentstore/rest_api/v0/views/xblock.py @@ -1,7 +1,14 @@ """ -Public rest API endpoints for the CMS API. +Public rest API endpoints for the CMS API — v0 xblock (DEPRECATED). + +.. deprecated:: + These views are superseded by ``XblockViewSet`` in + ``cms.djangoapps.contentstore.rest_api.v1.views.xblock``. + Use ``/api/contentstore/v1/xblock/`` going forward. + These v0 endpoints will be removed in a future release. """ import logging +import warnings from django.views.decorators.csrf import csrf_exempt from rest_framework.generics import CreateAPIView, RetrieveUpdateDestroyAPIView @@ -17,10 +24,17 @@ log = logging.getLogger(__name__) handle_xblock = view_handlers.handle_xblock +_DEPRECATION_MSG = ( + "The v0 xblock API (/api/contentstore/v0/xblock/) is deprecated. " + "Use /api/contentstore/v1/xblock/ instead." +) + @view_auth_classes() class XblockView(DeveloperErrorViewMixin, RetrieveUpdateDestroyAPIView): """ + **DEPRECATED** — use ``/api/contentstore/v1/xblock/{usage_key_string}/`` instead. + Public rest API endpoints for the CMS API. course_key: required argument, needed to authorize course authors. usage_key_string (optional): @@ -32,29 +46,35 @@ class XblockView(DeveloperErrorViewMixin, RetrieveUpdateDestroyAPIView): @course_author_access_required @expect_json_in_class_view def retrieve(self, request, course_key, usage_key_string=None): + warnings.warn(_DEPRECATION_MSG, DeprecationWarning, stacklevel=2) return handle_xblock(request, usage_key_string) @course_author_access_required @expect_json_in_class_view @validate_request_with_serializer def update(self, request, course_key, usage_key_string=None): + warnings.warn(_DEPRECATION_MSG, DeprecationWarning, stacklevel=2) return handle_xblock(request, usage_key_string) @course_author_access_required @expect_json_in_class_view @validate_request_with_serializer def partial_update(self, request, course_key, usage_key_string=None): + warnings.warn(_DEPRECATION_MSG, DeprecationWarning, stacklevel=2) return handle_xblock(request, usage_key_string) @course_author_access_required @expect_json_in_class_view def destroy(self, request, course_key, usage_key_string=None): + warnings.warn(_DEPRECATION_MSG, DeprecationWarning, stacklevel=2) return handle_xblock(request, usage_key_string) @view_auth_classes() class XblockCreateView(DeveloperErrorViewMixin, CreateAPIView): """ + **DEPRECATED** — use ``POST /api/contentstore/v1/xblock/`` instead. + Public rest API endpoints for the CMS API. course_key: required argument, needed to authorize course authors. usage_key_string (optional): @@ -68,4 +88,5 @@ class XblockCreateView(DeveloperErrorViewMixin, CreateAPIView): @expect_json_in_class_view @validate_request_with_serializer def create(self, request, course_key, usage_key_string=None): + warnings.warn(_DEPRECATION_MSG, DeprecationWarning, stacklevel=2) return handle_xblock(request, usage_key_string) diff --git a/cms/djangoapps/contentstore/rest_api/v1/urls.py b/cms/djangoapps/contentstore/rest_api/v1/urls.py index 685a81d778ce..48cd5118cc40 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/urls.py +++ b/cms/djangoapps/contentstore/rest_api/v1/urls.py @@ -2,6 +2,7 @@ from django.conf import settings from django.urls import path, re_path +from rest_framework.routers import DefaultRouter from openedx.core.constants import COURSE_ID_PATTERN @@ -27,6 +28,7 @@ ProctoringErrorsView, VideoDownloadView, VideoUsageView, + XblockViewSet, vertical_container_children_redirect_view, ) @@ -34,7 +36,10 @@ VIDEO_ID_PATTERN = r'(?P[-\w]+)' -urlpatterns = [ +_router = DefaultRouter() +_router.register(r'xblock', XblockViewSet, basename='xblock') + +urlpatterns = _router.urls + [ path( 'home', HomePageView.as_view(), diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py b/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py index 7654c9e0befc..f60e186f9f5c 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/__init__.py @@ -16,3 +16,4 @@ from .textbooks import CourseTextbooksView # noqa: F401 from .vertical_block import ContainerHandlerView, vertical_container_children_redirect_view # noqa: F401 from .videos import CourseVideosView, VideoDownloadView, VideoUsageView # noqa: F401 +from .xblock import XblockViewSet # noqa: F401 diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py b/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py new file mode 100644 index 000000000000..3b60607eebec --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/permissions.py @@ -0,0 +1,27 @@ +""" +Permission classes for v1 contentstore API views (ADR 0026). +""" +import logging + +from rest_framework.permissions import BasePermission + +from common.djangoapps.student.auth import has_course_author_access + +log = logging.getLogger(__name__) + + +class HasCourseAuthorAccess(BasePermission): + """ + ADR 0026: replaces the @course_author_access_required decorator. + + Reads ``view.kwargs["course_key"]`` (a CourseKey instance) that is + injected by XblockViewSet.initial() before DRF runs permission checks. + Returns 403 if the authenticated user lacks authoring rights on that + course, or if no course key could be derived. + """ + + def has_permission(self, request, view): + course_key = getattr(view, "course_key", None) + if not course_key: + return False + return has_course_author_access(request.user, course_key) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py new file mode 100644 index 000000000000..8694f7023683 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_xblock_viewset.py @@ -0,0 +1,144 @@ +""" +Tests for XblockViewSet (v1 — ADR 0028, 0026, 0029). + +Verifies: + * Each HTTP method routes to the correct per-verb handler (ADR 0028) + * Unauthenticated requests return standardized 401 (ADR 0029) + * Authenticated non-authors return standardized 403 (ADR 0029) + * ADR 0029 error envelope fields are present and correctly typed +""" +from unittest.mock import patch + +from django.http import JsonResponse +from django.urls import reverse +from rest_framework import status +from rest_framework.test import APITestCase + +from common.djangoapps.student.tests.factories import GlobalStaffFactory, UserFactory +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase + +TEST_LOCATOR = "block-v1:edX+ToyX+Toy_Course+type@problem+block@ba6327f840da49289fb27a9243913478" +PARENT_LOCATOR = "block-v1:edX+ToyX+Toy_Course+type@vertical+block@vert1" + +_REQUIRED_ERROR_FIELDS = ("type", "title", "status", "detail", "instance") + +_MOCK_RESPONSE = JsonResponse({"locator": TEST_LOCATOR}) + +_VIEW_MODULE = "cms.djangoapps.contentstore.rest_api.v1.views.xblock" + + +def _list_url(): + return reverse("cms.djangoapps.contentstore:v1:xblock-list") + + +def _detail_url(): + return reverse( + "cms.djangoapps.contentstore:v1:xblock-detail", + kwargs={"usage_key_string": TEST_LOCATOR}, + ) + + +# --------------------------------------------------------------------------- +# Routing tests +# --------------------------------------------------------------------------- + + +class XblockViewSetRoutingTest(ModuleStoreTestCase, APITestCase): + """Verify each HTTP method routes to the correct per-verb handler (ADR 0028).""" + + def setUp(self): + super().setUp() + self.staff = GlobalStaffFactory(password='password') + self.client.force_authenticate(user=self.staff) + + @patch(f"{_VIEW_MODULE}.create_xblock_response", return_value=_MOCK_RESPONSE) + def test_post_calls_create_xblock_response(self, mock_fn): + data = {"parent_locator": PARENT_LOCATOR, "category": "html"} + response = self.client.post(_list_url(), data=data, format="json") + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "POST" + + @patch(f"{_VIEW_MODULE}.retrieve_xblock_response", return_value=_MOCK_RESPONSE) + def test_get_calls_retrieve_xblock_response(self, mock_fn): + response = self.client.get(_detail_url()) + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "GET" + + @patch(f"{_VIEW_MODULE}.update_xblock_response", return_value=_MOCK_RESPONSE) + def test_put_calls_update_xblock_response(self, mock_fn): + data = {"id": TEST_LOCATOR, "data": "

Updated

"} + response = self.client.put(_detail_url(), data=data, format="json") + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "PUT" + + @patch(f"{_VIEW_MODULE}.update_xblock_response", return_value=_MOCK_RESPONSE) + def test_patch_calls_update_xblock_response(self, mock_fn): + data = {"id": TEST_LOCATOR, "display_name": "New Name"} + response = self.client.patch(_detail_url(), data=data, format="json") + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "PATCH" + + @patch(f"{_VIEW_MODULE}.delete_xblock_response", return_value=_MOCK_RESPONSE) + def test_delete_calls_delete_xblock_response(self, mock_fn): + response = self.client.delete(_detail_url()) + assert response.status_code == status.HTTP_200_OK + mock_fn.assert_called_once() + assert mock_fn.call_args[0][0].method == "DELETE" + + +# --------------------------------------------------------------------------- +# ADR 0029 error-shape tests +# --------------------------------------------------------------------------- + + +class XblockViewSetErrorShapeTest(ModuleStoreTestCase, APITestCase): + """Verify ADR 0029 standardized error envelope for auth failures.""" + + def setUp(self): + super().setUp() + self.non_author = UserFactory.create(password='password') + + def test_unauthenticated_returns_401(self): + response = self.client.get(_detail_url()) + assert response.status_code == status.HTTP_401_UNAUTHORIZED + + def test_unauthenticated_401_has_required_fields(self): + response = self.client.get(_detail_url()) + data = response.json() + for field in _REQUIRED_ERROR_FIELDS: + assert field in data, f"Missing ADR 0029 field: {field}" + + def test_unauthenticated_401_type_uri(self): + response = self.client.get(_detail_url()) + assert response.json()["type"] == "https://docs.openedx.org/errors/authn" + + def test_non_author_returns_403(self): + self.client.force_authenticate(user=self.non_author) + response = self.client.get(_detail_url()) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_non_author_403_has_required_fields(self): + self.client.force_authenticate(user=self.non_author) + response = self.client.get(_detail_url()) + data = response.json() + for field in _REQUIRED_ERROR_FIELDS: + assert field in data, f"Missing ADR 0029 field: {field}" + + def test_non_author_403_type_uri(self): + self.client.force_authenticate(user=self.non_author) + response = self.client.get(_detail_url()) + assert response.json()["type"] == "https://docs.openedx.org/errors/authz" + + def test_error_body_has_no_developer_message(self): + response = self.client.get(_detail_url()) + data = response.json() + assert "developer_message" not in data + assert "error_code" not in data + + def test_instance_field_is_request_path(self): + response = self.client.get(_detail_url()) + assert response.json()["instance"] == _detail_url() diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py new file mode 100644 index 000000000000..a3d2e38a3ba7 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/xblock.py @@ -0,0 +1,119 @@ +""" +API Views for Studio xblock CRUD — v1. + +Standardizes the v0 XblockView + XblockCreateView pair into a single +XblockViewSet applying the FC-0118 ADRs: + + * ADR 0025 - serializer_class + * ADR 0026 - explicit authentication_classes + permission_classes + * ADR 0028 - consolidated into XblockViewSet via DefaultRouter + * ADR 0029 - standardized error envelope via StandardizedErrorMixin +""" +import json +import logging + +from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication +from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import UsageKey +from rest_framework import viewsets +from rest_framework.permissions import IsAuthenticated + +from cms.djangoapps.contentstore.rest_api.mixins import StandardizedErrorMixin +from cms.djangoapps.contentstore.rest_api.v0.serializers import XblockSerializer +from cms.djangoapps.contentstore.rest_api.v0.views.utils import validate_request_with_serializer +from cms.djangoapps.contentstore.rest_api.v1.views.permissions import HasCourseAuthorAccess +from cms.djangoapps.contentstore.xblock_storage_handlers.view_handlers import ( + create_xblock_response, + delete_xblock_response, + retrieve_xblock_response, + update_xblock_response, +) +from common.djangoapps.util.json_request import expect_json_in_class_view + +log = logging.getLogger(__name__) + + +class XblockViewSet(StandardizedErrorMixin, viewsets.ViewSet): + """ + ViewSet for xblock CRUD operations (v1 — ADR 0028). + + Router-generated URLs: + POST /api/contentstore/v1/xblock/ → create + GET /api/contentstore/v1/xblock/{usage_key_string}/ → retrieve + PUT /api/contentstore/v1/xblock/{usage_key_string}/ → update + PATCH /api/contentstore/v1/xblock/{usage_key_string}/ → partial_update + DELETE /api/contentstore/v1/xblock/{usage_key_string}/ → destroy + """ + + authentication_classes = ( + JwtAuthentication, + SessionAuthenticationAllowInactiveUser, + ) + permission_classes = (IsAuthenticated, HasCourseAuthorAccess) + serializer_class = XblockSerializer + lookup_field = "usage_key_string" + lookup_value_regex = r'(?:i4x://?[^/]+/[^/]+/[^/]+/[^@]+(?:@[^/]+)?)|(?:[^/]+)' + + def __init__(self, **kwargs): + super().__init__(**kwargs) + self.course_key = None + + def initial(self, request, *args, **kwargs): + """ + Derive course_key and store it as self.course_key before DRF runs + permission checks. + + Detail actions (GET/PUT/PATCH/DELETE): course_key is extracted from + the usage key embedded in the URL. + Create action (POST): course_key is extracted from parent_locator in + the raw request body. We read request._request.body (Django's cached + bytes) rather than request.data to avoid consuming the WSGI stream + before @expect_json_in_class_view runs. + """ + usage_key_string = kwargs.get("usage_key_string") + if usage_key_string: + try: + self.course_key = UsageKey.from_string(usage_key_string).course_key + except InvalidKeyError: + self.course_key = None + else: + try: + # pylint: disable=protected-access + body = json.loads(request._request.body or b'{}') + parent_locator = body.get("parent_locator", "") + self.course_key = ( + UsageKey.from_string(parent_locator).course_key + if parent_locator else None + ) + except (ValueError, InvalidKeyError): + self.course_key = None + super().initial(request, *args, **kwargs) + + @expect_json_in_class_view + @validate_request_with_serializer + def create(self, request): + """Create a new xblock under the given parent.""" + return create_xblock_response(request) + + @expect_json_in_class_view + def retrieve(self, request, usage_key_string=None): + """Retrieve an xblock by its usage key.""" + return retrieve_xblock_response(request, usage_key_string) + + @expect_json_in_class_view + @validate_request_with_serializer + def update(self, request, usage_key_string=None): + """Fully update an xblock.""" + return update_xblock_response(request, usage_key_string) + + @expect_json_in_class_view + @validate_request_with_serializer + def partial_update(self, request, usage_key_string=None): + """Partially update an xblock.""" + return update_xblock_response(request, usage_key_string) + + @expect_json_in_class_view + def destroy(self, request, usage_key_string=None): + """Delete an xblock.""" + return delete_xblock_response(request, usage_key_string) diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 4f5e1ccb4244..e558c42735d1 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -305,46 +305,7 @@ def handle_xblock(request, usage_key_string=None): elif request.method in ("PUT", "POST"): if "duplicate_source_locator" in request.json: - parent_usage_key = usage_key_with_run(request.json["parent_locator"]) - duplicate_source_usage_key = usage_key_with_run( - request.json["duplicate_source_locator"] - ) - source_course = duplicate_source_usage_key.course_key - dest_course = parent_usage_key.course_key # noqa: F841 - - # Check authz permission for destination - permission = _check_xblock_permission(request, parent_usage_key) - # Legacy path also requires read access on the source course - if permission is None: - if not has_studio_read_access(request.user, source_course): - raise PermissionDenied() - - # Libraries have a maximum component limit enforced on them - if isinstance( - parent_usage_key, LibraryUsageLocator - ) and _is_library_component_limit_reached(parent_usage_key): - return JsonResponse( - { - "error": _( - "Libraries cannot have more than {limit} components" - ).format(limit=settings.MAX_BLOCKS_PER_CONTENT_LIBRARY) - }, - status=400, - ) - - dest_usage_key = duplicate_block( - parent_usage_key, - duplicate_source_usage_key, - request.user, - display_name=request.json.get('display_name'), - ) - - return JsonResponse( - { - "locator": str(dest_usage_key), - "courseKey": str(dest_usage_key.course_key), - } - ) + return _duplicate_xblock(request) else: return _create_block(request) elif request.method == "PATCH": @@ -378,6 +339,122 @@ def handle_xblock(request, usage_key_string=None): ) +# --------------------------------------------------------------------------- +# Public per-verb helpers for XblockViewSet (v1 — ADR 0028) +# These replace handle_xblock's internal method-dispatch so each ViewSet action +# calls the correct logic directly, satisfying REST semantics. +# --------------------------------------------------------------------------- + +def _duplicate_xblock(request): + """ + Shared duplicate-block logic used by both handle_xblock and create_xblock_response. + + Expects request.json to contain ``parent_locator`` and ``duplicate_source_locator``. + """ + parent_usage_key = usage_key_with_run(request.json["parent_locator"]) + duplicate_source_usage_key = usage_key_with_run( + request.json["duplicate_source_locator"] + ) + source_course = duplicate_source_usage_key.course_key + + # Check authz permission for destination + permission = _check_xblock_permission(request, parent_usage_key) + # Legacy path also requires read access on the source course + if permission is None: + if not has_studio_read_access(request.user, source_course): + raise PermissionDenied() + + # Libraries have a maximum component limit enforced on them + if isinstance( + parent_usage_key, LibraryUsageLocator + ) and _is_library_component_limit_reached(parent_usage_key): + return JsonResponse( + { + "error": _( + "Libraries cannot have more than {limit} components" + ).format(limit=settings.MAX_BLOCKS_PER_CONTENT_LIBRARY) + }, + status=400, + ) + + dest_usage_key = duplicate_block( + parent_usage_key, + duplicate_source_usage_key, + request.user, + display_name=request.json.get('display_name'), + ) + return JsonResponse( + { + "locator": str(dest_usage_key), + "courseKey": str(dest_usage_key.course_key), + } + ) + + +def create_xblock_response(request): + """ + Public entry point for POST (create). Called by XblockViewSet.create. + + Handles both duplication (duplicate_source_locator present) and normal creation. + """ + if "duplicate_source_locator" in request.json: + return _duplicate_xblock(request) + return _create_block_core(request) + + +def retrieve_xblock_response(request, usage_key_string): + """ + Public entry point for GET on a specific xblock. Called by XblockViewSet.retrieve. + """ + usage_key = usage_key_with_run(usage_key_string) + _check_xblock_permission(request, usage_key) + + accept_header = request.META.get("HTTP_ACCEPT", "application/json") + if "application/json" not in accept_header: + return HttpResponse(status=406) + + fields = request.GET.get("fields", "").split(",") + if "graderType" in fields: + return JsonResponse(CourseGradingModel.get_section_grader_type(usage_key)) + if "ancestorInfo" in fields: + xblock = get_xblock(usage_key, request.user) + return JsonResponse(_create_xblock_ancestor_info(xblock, is_concise=True)) + with modulestore().bulk_operations(usage_key.course_key): + response = get_block_info(get_xblock(usage_key, request.user)) + if "customReadToken" in fields: + parent_children = _get_block_parent_children(get_xblock(usage_key, request.user)) + response.update(parent_children) + return JsonResponse(response) + + +def update_xblock_response(request, usage_key_string): + """ + Public entry point for PUT/PATCH on a specific xblock. Called by XblockViewSet.update/partial_update. + + For PATCH with ``move_source_locator``, routes to the move-item path (permission check + is against the target parent, not the URL's usage key). + """ + if request.method == "PATCH" and "move_source_locator" in request.json: + move_source_usage_key = usage_key_with_run(request.json.get("move_source_locator")) + target_parent_usage_key = usage_key_with_run(request.json.get("parent_locator")) + target_index = request.json.get("target_index") + _check_xblock_permission(request, target_parent_usage_key) + return _move_item(move_source_usage_key, target_parent_usage_key, request.user, target_index) + usage_key = usage_key_with_run(usage_key_string) + _check_xblock_permission(request, usage_key) + return modify_xblock(usage_key, request) + + +def delete_xblock_response(request, usage_key_string): + """ + Public entry point for DELETE on a specific xblock. Called by XblockViewSet.destroy. + """ + usage_key = usage_key_with_run(usage_key_string) + _check_xblock_permission(request, usage_key) + _delete_item(usage_key, request.user) + return JsonResponse() + + def modify_xblock(usage_key, request): request_data = request.json return _save_xblock( @@ -746,10 +823,12 @@ def sync_library_content( return static_file_notices -@login_required -@expect_json -def _create_block(request): - """View for create blocks.""" +def _create_block_core(request): + """ + Core xblock creation logic, usable without @login_required / @expect_json decorators. + + Called by both _create_block (legacy view) and create_xblock_response (v1 ViewSet). + """ parent_locator = request.json["parent_locator"] usage_key = usage_key_with_run(parent_locator) category = request.json.get("category") @@ -836,6 +915,13 @@ def _create_block(request): return JsonResponse(response) +@login_required +@expect_json +def _create_block(request): + """View for create blocks.""" + return _create_block_core(request) + + def _get_source_index(source_usage_key, source_parent): """ Get source index position of the XBlock.