From eab3d42a0a30bae4f2d098516869e6874957162d Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Mon, 5 Oct 2026 00:46:01 +0500 Subject: [PATCH 1/7] feat: publish full CMS schema paths and register path converters The Studio schema trimmed /api/contentstore from every path and offered a server that re-added it, so an operation under any other API name could not be published at an address a client can call. Publish every path in full against the service-root servers and drop the contentstore server. SCHEMA_PATH_PREFIX stays, so every existing operationId and tag is unchanged. Admit /api/authoring/ in the pre-processing filter, and add a post-processing hook that marks the operations listed in SUPERSEDED_PATH_PREFIXES deprecated. The list starts empty; each successor adds its entry in the change that mounts it. Register the shared opaque-key path converters before autodiscovery, so routes can parse course keys in the path rather than in the view. Part of #39071. --- .../tests/test_video_management_contract.py | 193 ++++++++++++++++++ cms/envs/devstack.py | 15 +- cms/envs/production.py | 15 +- cms/lib/spectacular.py | 24 ++- cms/urls.py | 4 + 5 files changed, 240 insertions(+), 11 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py new file mode 100644 index 000000000000..86562175050c --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py @@ -0,0 +1,193 @@ +""" +Contract tests shared by the course video APIs. +""" +import functools +import json +import os +import subprocess +import sys +from pathlib import Path + +import ddt +from django.test import TestCase +from drf_spectacular.generators import SchemaGenerator +from drf_spectacular.settings import patched_settings + +import cms.envs +from cms.lib.spectacular import cms_api_filter + +LEGACY_USAGE_PATH = "/api/contentstore/v1/videos/{course_id}/{edx_video_id}/usage" +LEGACY_DOWNLOAD_PATH = "/api/contentstore/v1/videos/{course_id}/download" + +ENUM_POSTPROCESSING_HOOK = "drf_spectacular.hooks.postprocess_schema_enums" +MIGRATED_PATHS_HOOK = "cms.lib.spectacular.cms_mark_migrated_paths" + +PRODUCTION_SETTINGS = "cms.envs.production" +DEVSTACK_SETTINGS = "cms.envs.devstack" +SETTINGS_DIRECTORY = Path(cms.envs.__file__).resolve().parent +REPO_ROOT = SETTINGS_DIRECTORY.parents[1] +MOCK_CONFIG = SETTINGS_DIRECTORY / "mock.yml" + +# The schema settings a document generated in this process has to be given for +# its addresses to come out the way the deployment publishes them. +GENERATION_SETTING_KEYS = ("PREPROCESSING_HOOKS", "POSTPROCESSING_HOOKS", "SCHEMA_PATH_PREFIX") +SETTINGS_MARKER = "schema settings: " + + +@functools.lru_cache +def schema_settings(module): + """ + Return ``SPECTACULAR_SETTINGS`` of the deployment settings module ``module``. + + Deployment settings share their mutable defaults with the settings the test + process runs under and would alter them on import, so they are read in a + separate process. + """ + script = ( + "import importlib, json, sys\n" + "values = importlib.import_module(sys.argv[1]).SPECTACULAR_SETTINGS\n" + "print(sys.argv[2] + json.dumps(values))\n" + ) + completed = subprocess.run( + [sys.executable, "-c", script, module, SETTINGS_MARKER], + capture_output=True, + check=True, + cwd=REPO_ROOT, + env={**os.environ, "CMS_CFG": str(MOCK_CONFIG), "SERVICE_VARIANT": "cms"}, + text=True, + ) + reported = next(line for line in completed.stdout.splitlines() if line.startswith(SETTINGS_MARKER)) + return json.loads(reported[len(SETTINGS_MARKER):]) + + +def generate_document(postprocessing_hooks=None, generator=None): + """Generate the Studio document with the production schema settings.""" + configured = schema_settings(PRODUCTION_SETTINGS) + overrides = {key: configured[key] for key in GENERATION_SETTING_KEYS} + if postprocessing_hooks is not None: + overrides["POSTPROCESSING_HOOKS"] = postprocessing_hooks + with patched_settings(overrides): + return (generator or SchemaGenerator()).get_schema(request=None, public=True) + + +@functools.lru_cache +def production_document(): + return generate_document() + + +def operations(document): + """Yield (path, method, operation) for every operation of ``document``.""" + for path, item in document["paths"].items(): + for method, operation in item.items(): + if isinstance(operation, dict) and "operationId" in operation: + yield path, method, operation + + +class CmsSchemaHookTest(TestCase): + """The schema hooks admit the new addresses.""" + + def test_filter_admits_the_authoring_prefix(self): + admitted = cms_api_filter([ + ("/api/authoring/v2/courses/course-v1:a+b+c/video_archives/", None, "post", None), + ("/api/contentstore/v1/videos/course-v1:a+b+c/download", None, "put", None), + ("/api/authoring/v2", None, "get", None), + ("/login", None, "get", None), + ]) + + assert [entry[0] for entry in admitted] == [ + "/api/authoring/v2/courses/course-v1:a+b+c/video_archives/", + "/api/contentstore/v1/videos/course-v1:a+b+c/download", + ] + + def test_filter_refuses_the_versioned_paths_of_other_apis(self): + admitted = cms_api_filter([ + ("/api/xblock/v2/xblocks/lb:Org:lib:html:abc/", None, "get", None), + ("/api/content_tagging/v1/taxonomies/", None, "get", None), + ("/api/libraries/v2/", None, "get", None), + ("/api/courses/v1/courses/", None, "get", None), + ("/api/authoring_tools/v1/", None, "get", None), + ("/api/v1/authoring/", None, "get", None), + ]) + + assert not admitted, admitted + + def test_filter_admits_the_course_discussions_switch_and_nothing_else_under_courses(self): + admitted = cms_api_filter([ + ("/api/courses/{course_key_string}/bulk_enable_disable_discussions", None, "put", None), + ("/api/courses/{course_key_string}/updates", None, "get", None), + ("/api/discussions/v0/bulk_enable_disable_discussions", None, "get", None), + ]) + + assert [entry[0] for entry in admitted] == [ + "/api/courses/{course_key_string}/bulk_enable_disable_discussions", + ] + + +class CmsSchemaDocumentTest(TestCase): + """The published Studio document, generated with the production schema settings.""" + + def test_only_the_studio_apis_are_published(self): + paths = list(production_document()["paths"]) + + assert {path.split("/")[2] for path in paths} == {"contentstore", "courses"} + assert [path for path in paths if path.startswith("/api/courses/")] == [ + "/api/courses/{course_key_string}/bulk_enable_disable_discussions", + ] + + def test_operation_ids(self): + document = production_document() + + assert document["paths"][LEGACY_USAGE_PATH]["get"]["operationId"] == "v1_videos_usage_retrieve" + assert document["paths"][LEGACY_DOWNLOAD_PATH]["put"]["operationId"] == "v1_videos_download_update" + + @staticmethod + def marks(document, path, method): + operation = document["paths"][path][method] + return operation.get("deprecated"), operation.get("x-internal") + + def test_no_operation_is_flagged_before_its_successor_exists(self): + document = production_document() + + flagged = [ + (path, method) + for path, method, __ in operations(document) + if self.marks(document, path, method) != (None, None) + ] + assert flagged == [] + + def test_enum_components_survive_the_registered_post_processing(self): + without_enum_hook = generate_document(postprocessing_hooks=[MIGRATED_PATHS_HOOK]) + + assert not [name for name in without_enum_hook["components"]["schemas"] if name.endswith("Enum")] + assert "ContentTypeEnum" in production_document()["components"]["schemas"] + + +@ddt.ddt +class CmsSchemaSettingsTest(TestCase): + """The deployment settings publish full paths, and no server adds an API prefix of its own.""" + + @ddt.data(PRODUCTION_SETTINGS, DEVSTACK_SETTINGS) + def test_paths_are_published_in_full(self, module): + configured = schema_settings(module) + + assert "SCHEMA_PATH_PREFIX_TRIM" not in configured + assert configured["SCHEMA_PATH_PREFIX"] == r"/api/(contentstore|authoring)" + + @ddt.data(PRODUCTION_SETTINGS, DEVSTACK_SETTINGS) + def test_the_post_processing_hooks_keep_the_enum_hook(self, module): + hooks = schema_settings(module)["POSTPROCESSING_HOOKS"] + + assert [hook for hook in hooks if hook in (ENUM_POSTPROCESSING_HOOK, MIGRATED_PATHS_HOOK)] == [ + ENUM_POSTPROCESSING_HOOK, + MIGRATED_PATHS_HOOK, + ] + + # The Public server is AUTHORING_API_URL as configured. Whether a gateway + # behind it serves the full paths is not decided by these settings and is + # not checked here. + @ddt.data(PRODUCTION_SETTINGS, DEVSTACK_SETTINGS) + def test_no_server_url_carries_an_api_prefix(self, module): + servers = schema_settings(module)["SERVERS"] + + assert [server["description"] for server in servers] == ["Public", "Local"] + assert [server for server in servers if "/api/" in server["url"]] == [] diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index 0576a35272af..df08d196def3 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -356,12 +356,19 @@ def should_show_debug_toolbar(request): # pylint: disable=missing-function-docs 'SERVE_INCLUDE_SCHEMA': False, # restrict spectacular to CMS API endpoints (cms/lib/spectacular.py): 'PREPROCESSING_HOOKS': ['cms.lib.spectacular.cms_api_filter'], - # remove the default schema path prefix to replace it with server-specific base paths: - 'SCHEMA_PATH_PREFIX': '/api/contentstore', - 'SCHEMA_PATH_PREFIX_TRIM': '/api/contentstore', + # Mark superseded legacy addresses deprecated. + # The enum hook is drf-spectacular's default, restated because setting + # this key replaces the default list. + 'POSTPROCESSING_HOOKS': [ + 'drf_spectacular.hooks.postprocess_schema_enums', + 'cms.lib.spectacular.cms_mark_migrated_paths', + ], + # Stripped from each path to derive its tags and operationId, so changing + # it renames every operationId. Paths themselves are emitted in full so + # they resolve against the service-root SERVERS below. + 'SCHEMA_PATH_PREFIX': r'/api/(contentstore|authoring)', 'SERVERS': [ {'url': AUTHORING_API_URL, 'description': 'Public'}, # noqa: F405 {'url': f'http://{CMS_BASE}', 'description': 'Local'}, - {'url': f'http://{CMS_BASE}/api/contentstore', 'description': 'CMS-contentstore'} ], } diff --git a/cms/envs/production.py b/cms/envs/production.py index b15caa2d1c14..decb3a0f09b0 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -415,13 +415,20 @@ def get_env_setting(setting): 'SERVE_INCLUDE_SCHEMA': False, # restrict spectacular to CMS API endpoints (cms/lib/spectacular.py): 'PREPROCESSING_HOOKS': ['cms.lib.spectacular.cms_api_filter'], - # remove the default schema path prefix to replace it with server-specific base paths: - 'SCHEMA_PATH_PREFIX': '/api/contentstore', - 'SCHEMA_PATH_PREFIX_TRIM': '/api/contentstore', + # Mark superseded legacy addresses deprecated. + # The enum hook is drf-spectacular's default, restated because setting + # this key replaces the default list. + 'POSTPROCESSING_HOOKS': [ + 'drf_spectacular.hooks.postprocess_schema_enums', + 'cms.lib.spectacular.cms_mark_migrated_paths', + ], + # Stripped from each path to derive its tags and operationId, so changing + # it renames every operationId. Paths themselves are emitted in full so + # they resolve against the service-root SERVERS below. + 'SCHEMA_PATH_PREFIX': r'/api/(contentstore|authoring)', 'SERVERS': [ {'url': AUTHORING_API_URL, 'description': 'Public'}, # noqa: F405 {'url': f'https://{CMS_BASE}', 'description': 'Local'}, # noqa: F405 - {'url': f'https://{CMS_BASE}/api/contentstore', 'description': 'CMS-contentstore'} # noqa: F405 ], } diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index 90bce5668fec..1c004d442b91 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -2,14 +2,18 @@ import re +# Path prefixes of operations superseded by a newer version, kept for their +# deprecation window. +SUPERSEDED_PATH_PREFIXES = () + def cms_api_filter(endpoints): """ - Pre-processing hook: keep only contentstore versioned endpoints and select - course-level endpoints. + Pre-processing hook: keep only contentstore + authoring versioned + endpoints and select course-level endpoints. """ filtered = [] - CMS_PATH_PATTERN = re.compile(r"^/api/contentstore/v\d+/") + CMS_PATH_PATTERN = re.compile(r"^/api/(contentstore|authoring)/v\d+/") for path, path_regex, method, callback in endpoints: if ( @@ -22,3 +26,17 @@ def cms_api_filter(endpoints): filtered.append((path, path_regex, method, callback)) return filtered + + +def cms_mark_migrated_paths(result, generator, request, public): # pylint: disable=unused-argument + """ + Post-processing hook: mark the operations that have a conforming successor + ``deprecated: true``. + """ + for path, path_item in result.get("paths", {}).items(): + if not path.startswith(SUPERSEDED_PATH_PREFIXES): + continue + for operation in path_item.values(): + if isinstance(operation, dict): + operation["deprecated"] = True + return result diff --git a/cms/urls.py b/cms/urls.py index c0f96f489bb8..f8140f15422f 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -13,6 +13,7 @@ from django.views.generic import RedirectView from drf_spectacular.views import SpectacularAPIView, SpectacularSwaggerView from edx_api_doc_tools import make_docs_urls +from edx_rest_framework_extensions.url_converters import register_url_converters import openedx.core.djangoapps.common_views.xblock import openedx.core.djangoapps.debug.views @@ -26,6 +27,9 @@ from openedx.core.djangoapps.password_policy import compliance as password_policy_compliance from openedx.core.djangoapps.password_policy.forms import PasswordPolicyAwareAdminAuthForm +# Shared opaque-key path converters, registered before any pattern using them. +register_url_converters() + django_autodiscover() admin.site.site_header = _('Studio Administration') admin.site.site_title = admin.site.site_header From 0ae573baf02770dbf795d49e92ced1f6c0960372 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Mon, 5 Oct 2026 00:46:15 +0500 Subject: [PATCH 2/7] feat: add a v2 video usages endpoint to the authoring API GET /api/authoring/v2/courses/{course_key}/video_usages/{edx_video_id}/ returns where a video is used in a course, with the same body as GET /api/contentstore/v1/videos/{course_id}/{edx_video_id}/usage. The v1 endpoint answers errors as {developer_message, error_code}, accepts Bearer tokens and inactive session users, and checks access inline after looking up the course; changing any of that in place would break its callers. The new endpoint uses the standard error envelope, the platform's default authentication, and view-level permissions (IsAuthenticated, HasFullCourseKey, HasStudioReadAccess) that run before the course lookup, so a refusal no longer reveals whether a course exists. Addresses under the collection that no route serves, including course keys the converter refuses, answer with the JSON error envelope rather than Studio's HTML 404. The v1 operation is marked deprecated in the schema; its view is unchanged. Part of #39071. --- .../rest_api/v1/views/unknown_route.py | 25 + .../rest_api/v2/authoring_urls.py | 35 + .../rest_api/v2/serializers/video_usages.py | 34 + .../rest_api/v2/video_management_service.py | 30 + .../tests/test_video_management_contract.py | 376 +++++++- .../v2/views/tests/test_video_usages.py | 844 ++++++++++++++++++ .../rest_api/v2/views/video_schema.py | 47 + .../rest_api/v2/views/video_usages.py | 97 ++ .../contentstore/views/permissions.py | 36 +- cms/lib/spectacular.py | 5 +- cms/urls.py | 4 + 11 files changed, 1519 insertions(+), 14 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/authoring_urls.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/serializers/video_usages.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/video_management_service.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_usages.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/video_schema.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/video_usages.py diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py b/cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py new file mode 100644 index 000000000000..e551794a8507 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v1/views/unknown_route.py @@ -0,0 +1,25 @@ +"""API view answering addresses that no endpoint serves.""" + +from drf_spectacular.utils import extend_schema +from edx_rest_framework_extensions.mixins import StandardizedErrorMixin +from rest_framework.exceptions import NotFound +from rest_framework.permissions import AllowAny +from rest_framework.views import APIView + + +@extend_schema(exclude=True) +class UnknownRouteView(StandardizedErrorMixin, APIView): + """ + Answer a video address whose course key the conforming routes refuse. + + A key that is malformed or in the deprecated slash-separated form fails to + resolve, and would otherwise be answered by the site's HTML error page, which + an API client cannot read. Nothing here needs the caller's identity, so the + answer is the same for everyone: the address holds no resource. + """ + + permission_classes = (AllowAny,) + + def initial(self, request, *args, **kwargs): + """Refuse the request before any handler runs; the address holds nothing.""" + raise NotFound() diff --git a/cms/djangoapps/contentstore/rest_api/v2/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v2/authoring_urls.py new file mode 100644 index 000000000000..82cd1802dc87 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/authoring_urls.py @@ -0,0 +1,35 @@ +"""Contentstore API v2 authoring URLs.""" + +from django.urls import path, re_path + +from cms.djangoapps.contentstore.rest_api.v1.views.unknown_route import UnknownRouteView +from cms.djangoapps.contentstore.rest_api.v2.views.video_usages import CourseVideoUsageViewSet + +app_name = "authoring_v2" + +urlpatterns = [ + path( + "courses//video_usages//", + CourseVideoUsageViewSet.as_view({"get": "retrieve"}), + name="course_video_usage_detail", + ), + # Every other address under this collection holds nothing: a course key + # the route above refuses (malformed, or in the deprecated slash-separated + # form), the usage collection without a video id, an empty video id, or a + # path below a usage. They answer with the API error body rather than the + # site's HTML error page. They match addresses under this collection and + # nothing else, so a resource added under courses/ is never shadowed, and + # only slash-terminated ones, so the redirect to the slash-terminated form + # still happens first. A path converter cannot match an empty segment, + # hence the pattern for the addresses below a usage. + path( + "courses//video_usages/", + UnknownRouteView.as_view(), + name="course_video_usage_list_unmatched", + ), + re_path( + r"^courses/(?P.+)/video_usages/(?P.*)/$", + UnknownRouteView.as_view(), + name="course_video_usage_detail_unmatched", + ), +] diff --git a/cms/djangoapps/contentstore/rest_api/v2/serializers/video_usages.py b/cms/djangoapps/contentstore/rest_api/v2/serializers/video_usages.py new file mode 100644 index 000000000000..c4862d114463 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/serializers/video_usages.py @@ -0,0 +1,34 @@ +"""Serializers for where a video is used in a course.""" + +from rest_framework import serializers + + +class VideoUsageLocationSerializer(serializers.Serializer): # pylint: disable=abstract-method + """One video component of the course that plays the video.""" + + display_location = serializers.CharField( + help_text=( + "Where the component sits, as ' - / ' display names. " + "A subsection or unit with no display name set appears as 'None', and a component with " + "none as 'Video'. The subsection part is empty for a unit that is in no subsection." + ), + ) + url = serializers.CharField( + help_text=( + "Studio address of the unit holding the component, anchored at the component: " + "'/container/#'." + ), + ) + + +class CourseVideoUsageSerializer(serializers.Serializer): # pylint: disable=abstract-method + """The places in a course where one video is used.""" + + usage_locations = VideoUsageLocationSerializer( + many=True, + help_text=( + "One entry per video component that references the video, published or not. A " + "component with no parent in the course outline is left out. Empty when no component " + "is listed." + ), + ) diff --git a/cms/djangoapps/contentstore/rest_api/v2/video_management_service.py b/cms/djangoapps/contentstore/rest_api/v2/video_management_service.py new file mode 100644 index 000000000000..30c53dd10a53 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/video_management_service.py @@ -0,0 +1,30 @@ +"""Service layer for the video usage resources of a course.""" + +from rest_framework.exceptions import NotFound + +from cms.djangoapps.contentstore.video_storage_handlers import get_video_usage_path +from openedx.core.djangoapps.content.course_overviews.models import CourseOverview + + +def ensure_course_exists(course_key): + """ + Raise ``NotFound`` unless a course run exists under ``course_key``. + + Only reads: the course overview table first, then the content store. The + overview row is never created here, so a read request writes nothing. + """ + if not CourseOverview.course_exists(course_key): + raise NotFound("The course does not exist.") + + +def get_course_video_usages(course_key, edx_video_id): + """ + Return where the video ``edx_video_id`` is placed in the course. + + The result is ``{"usage_locations": [{"display_location", "url"}, ...]}``, + one entry per video component that references the id, in content-store + order. Unpublished components are included; a component with no parent is + left out. An id that nothing references gives an empty list. + """ + ensure_course_exists(course_key) + return get_video_usage_path(course_key, edx_video_id) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py index 86562175050c..803e0fed0617 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py @@ -6,16 +6,36 @@ import os import subprocess import sys +from collections import defaultdict from pathlib import Path +from types import SimpleNamespace import ddt -from django.test import TestCase +import pytest +from django.contrib.auth.models import AnonymousUser +from django.test import RequestFactory, TestCase +from django.urls import Resolver404, get_resolver, resolve, reverse from drf_spectacular.generators import SchemaGenerator from drf_spectacular.settings import patched_settings +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey +from rest_framework.exceptions import NotFound import cms.envs -from cms.lib.spectacular import cms_api_filter - +from cms.djangoapps.contentstore.rest_api.v2.serializers.video_usages import VideoUsageLocationSerializer +from cms.djangoapps.contentstore.rest_api.v2.views.video_usages import CourseVideoUsageViewSet +from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from cms.djangoapps.contentstore.views.permissions import HasFullCourseKey, HasStudioReadAccess +from cms.lib.spectacular import SUPERSEDED_PATH_PREFIXES, cms_api_filter, cms_mark_migrated_paths +from common.djangoapps.student.roles import ( + CourseInstructorRole, + CourseLimitedStaffRole, + CourseStaffRole, +) +from common.djangoapps.student.tests.factories import UserFactory +from xmodule.modulestore.tests.factories import CourseFactory + +USAGE_PATH = "/api/authoring/v2/courses/{course_key}/video_usages/{edx_video_id}/" LEGACY_USAGE_PATH = "/api/contentstore/v1/videos/{course_id}/{edx_video_id}/usage" LEGACY_DOWNLOAD_PATH = "/api/contentstore/v1/videos/{course_id}/download" @@ -33,6 +53,15 @@ GENERATION_SETTING_KEYS = ("PREPROCESSING_HOOKS", "POSTPROCESSING_HOOKS", "SCHEMA_PATH_PREFIX") SETTINGS_MARKER = "schema settings: " +# Keys the course_key path converter accepts that carry only a content version, +# with no organization, course or run. +VERSION_ONLY_COURSE_KEYS = ( + "course-v1:version@" + "a" * 24, + "course-v1:branch@published-branch+version@" + "a" * 24, + "ccx-v1:version@" + "a" * 24 + "+ccx@1", + "library-v1:version@" + "a" * 24, +) + @functools.lru_cache def schema_settings(module): @@ -83,8 +112,165 @@ def operations(document): yield path, method, operation +def record_assigned_operation_ids(result, generator, request, public): # pylint: disable=unused-argument + """ + Post-processing hook: keep every operation's id on the generator, unchanged. + + Post-processing runs before the generator renames colliding ids with numeric + suffixes, so the ids kept here are the ones each path was given. + """ + generator.assigned_operation_ids = [ + (operation["operationId"], path, method) for path, method, operation in operations(result) + ] + return result + + +RECORDING_HOOK = f"{record_assigned_operation_ids.__module__}.{record_assigned_operation_ids.__name__}" + + +class VideoRoutesTest(TestCase): + """Route names: the legacy ones stay as they are; the new ones are unique.""" + + COURSE_KEY = "course-v1:edX+DemoX+Demo_Course" + + # The routes of this resource, in order. Other resources share the + # namespace, so only these are pinned. + ROUTE_NAMES = ( + "course_video_usage_detail", + "course_video_usage_list_unmatched", + "course_video_usage_detail_unmatched", + ) + + # Addresses outside the collection, some named like it, that another + # resource may serve. Neither fallback may claim them. + ADDRESSES_OUTSIDE_THE_COLLECTION = ( + f"/api/authoring/v2/courses/{COURSE_KEY}/", + f"/api/authoring/v2/courses/{COURSE_KEY}/textbooks/", + "/api/authoring/v2/courses/Org/Course/Run/textbooks/", + f"/api/authoring/v2/courses/{COURSE_KEY}/textbooks/video_usages_report/", + f"/api/authoring/v2/courses/{COURSE_KEY}/video_usage/abc-123/", + "/api/authoring/v2/video_usages/", + ) + + def test_the_shared_legacy_name_without_a_video_id_reverses_to_the_download_route(self): + address = reverse("cms.djangoapps.contentstore:v1:video_usage", kwargs={"course_id": self.COURSE_KEY}) + + assert address == f"/api/contentstore/v1/videos/{self.COURSE_KEY}/download" + assert resolve(address).func.view_class.__name__ == "VideoDownloadView" + + def test_the_shared_legacy_name_with_a_video_id_reverses_to_the_usage_route(self): + address = reverse( + "cms.djangoapps.contentstore:v1:video_usage", + kwargs={"course_id": self.COURSE_KEY, "edx_video_id": "abc-123"}, + ) + + assert address == f"/api/contentstore/v1/videos/{self.COURSE_KEY}/abc-123/usage" + assert resolve(address).func.view_class.__name__ == "VideoUsageView" + + def test_the_new_route_names_are_unique_and_carry_no_version(self): + __, resolver = get_resolver().namespace_dict["authoring_v2"] + + names = [pattern.name for pattern in resolver.url_patterns if pattern.name in self.ROUTE_NAMES] + + # Each fallback route has to come after the route it backs, or it would + # answer valid course keys too. + assert names == list(self.ROUTE_NAMES) + assert resolver.app_name == "authoring_v2" + assert not [name for name in names if "v1" in name or "v2" in name] + + def test_the_fallbacks_claim_nothing_outside_the_collection(self): + fallbacks = {f"authoring_v2:{name}" for name in self.ROUTE_NAMES if name.endswith("_unmatched")} + claimed = [] + for address in self.ADDRESSES_OUTSIDE_THE_COLLECTION: + try: + match = resolve(address) + except Resolver404: + continue + if match.view_name in fallbacks: + claimed.append(address) + + assert not claimed, claimed + + +class HasStudioReadAccessTest(CourseTestCase): + """The permission grants exactly the callers with Studio read access to the course in the path.""" + + def allows(self, user, course_key=None, kwargs=None): + request = RequestFactory().get("/") + request.user = user + view = SimpleNamespace(kwargs={"course_key": course_key or self.course.id} if kwargs is None else kwargs) + return HasStudioReadAccess().has_permission(request, view) + + def user_with_role(self, role_class, course_key=None): + user = UserFactory.create() + role_class(course_key or self.course.id).add_users(user) + return user + + def test_global_staff_is_allowed(self): + assert self.allows(self.user) is True + + def test_course_staff_is_allowed(self): + assert self.allows(self.user_with_role(CourseStaffRole)) is True + + def test_course_instructor_is_allowed(self): + assert self.allows(self.user_with_role(CourseInstructorRole)) is True + + def test_user_without_a_role_is_refused(self): + assert self.allows(UserFactory.create()) is False + + def test_anonymous_is_refused(self): + assert self.allows(AnonymousUser()) is False + + def test_limited_staff_is_refused(self): + assert self.allows(self.user_with_role(CourseLimitedStaffRole)) is False + + def test_staff_of_another_course_is_refused(self): + other_course = CourseFactory.create() + + assert self.allows(self.user_with_role(CourseStaffRole, course_key=other_course.id)) is False + + def test_ccx_course_is_refused_to_global_staff(self): + assert self.allows(self.user, course_key=CourseKey.from_string("ccx-v1:edX+DemoX+Demo_Course+ccx@1")) is False + + def test_a_view_without_a_course_key_is_refused(self): + assert self.allows(self.user, kwargs={}) is False + + # The role checks behind this class cannot evaluate a key that carries only + # a content version, which is why the views list HasFullCourseKey first. + def test_a_key_without_an_organization_cannot_be_evaluated(self): + with pytest.raises(InvalidKeyError): + self.allows(self.user, course_key=CourseKey.from_string(VERSION_ONLY_COURSE_KEYS[0])) + + +@ddt.ddt +class HasFullCourseKeyTest(TestCase): + """The permission answers 404 for a course key with no organization, course or run, and passes any other.""" + + def check(self, kwargs): + request = RequestFactory().get("/") + request.user = AnonymousUser() + return HasFullCourseKey().has_permission(request, SimpleNamespace(kwargs=kwargs)) + + @ddt.data(*VERSION_ONLY_COURSE_KEYS) + def test_a_key_carrying_only_a_version_is_not_found(self, key): + with pytest.raises(NotFound): + self.check({"course_key": CourseKey.from_string(key)}) + + @ddt.data( + "course-v1:edX+DemoX+Demo_Course", + "course-v1:edX+DemoX+Demo_Course+version@" + "a" * 24, + "ccx-v1:edX+DemoX+Demo_Course+ccx@1", + "library-v1:edX+DemoLib", + ) + def test_a_key_naming_an_organization_passes(self, key): + assert self.check({"course_key": CourseKey.from_string(key)}) is True + + def test_a_view_without_a_course_key_passes(self): + assert self.check({}) is True + + class CmsSchemaHookTest(TestCase): - """The schema hooks admit the new addresses.""" + """The schema hooks admit the new addresses and deprecate exactly the superseded one.""" def test_filter_admits_the_authoring_prefix(self): admitted = cms_api_filter([ @@ -122,38 +308,204 @@ def test_filter_admits_the_course_discussions_switch_and_nothing_else_under_cour "/api/courses/{course_key_string}/bulk_enable_disable_discussions", ] + def test_the_superseded_paths_include_the_usage_operation(self): + assert [ + prefix for prefix in SUPERSEDED_PATH_PREFIXES if prefix in (LEGACY_USAGE_PATH, LEGACY_DOWNLOAD_PATH) + ] == [LEGACY_USAGE_PATH] + + # Operations of other resources that nothing supersedes, next to the + # superseded one or elsewhere in Studio, and the new address. Marking any + # of them would tell clients to leave an operation that has no successor. + # Resources superseded in their own right are not listed: whether they are + # marked is decided with their successors, not here. + OTHER_RESOURCE_PATHS = ( + "/api/contentstore/v1/videos/{course_id}", + LEGACY_DOWNLOAD_PATH, + "/api/contentstore/v0/videos/encodings/{course_id}", + "/api/contentstore/v0/videos/features", + "/api/contentstore/v0/videos/images/{course_id}/{edx_video_id}", + "/api/contentstore/v2/home/courses", + "/api/contentstore/v2/downstreams/{usage_key_string}", + USAGE_PATH, + ) + + def marked_schema(self): + """Run the post-processing hook over the superseded path and those of other resources.""" + paths = { + LEGACY_USAGE_PATH: {"get": {}}, + } + paths.update({path: {"get": {}, "post": {}} for path in self.OTHER_RESOURCE_PATHS}) + return cms_mark_migrated_paths({"paths": paths}, None, None, True) + + def test_post_processing_marks_the_superseded_operation_deprecated(self): + paths = self.marked_schema()["paths"] + + assert paths[LEGACY_USAGE_PATH] == {"get": {"deprecated": True}} + + def test_post_processing_leaves_other_resources_alone(self): + paths = self.marked_schema()["paths"] + assert {path: paths[path] for path in self.OTHER_RESOURCE_PATHS} == { + path: {"get": {}, "post": {}} for path in self.OTHER_RESOURCE_PATHS + } + + def test_post_processing_leaves_other_members_of_a_path_alone(self): + result = cms_mark_migrated_paths( + {"paths": {LEGACY_USAGE_PATH: {"get": {}, "parameters": []}}}, None, None, True, + ) + + assert result["paths"][LEGACY_USAGE_PATH] == {"get": {"deprecated": True}, "parameters": []} + + +@ddt.ddt class CmsSchemaDocumentTest(TestCase): """The published Studio document, generated with the production schema settings.""" + def test_the_new_operation_is_published_at_its_full_address(self): + document = production_document() + + assert [method for method in document["paths"][USAGE_PATH] if method != "parameters"] == ["get"] + def test_only_the_studio_apis_are_published(self): paths = list(production_document()["paths"]) - assert {path.split("/")[2] for path in paths} == {"contentstore", "courses"} + assert {path.split("/")[2] for path in paths} == {"contentstore", "authoring", "courses"} assert [path for path in paths if path.startswith("/api/courses/")] == [ "/api/courses/{course_key_string}/bulk_enable_disable_discussions", ] + def test_no_other_address_of_these_resources_is_published(self): + published = { + path + for path in production_document()["paths"] + if path.startswith("/api/authoring/") and {"video_usages", "video_archives"} & set(path.split("/")) + } + + assert published == {USAGE_PATH} + + def test_the_published_address_resolves_to_the_new_view(self): + usage = resolve(USAGE_PATH.format(course_key="course-v1:a+b+c", edx_video_id="v1")) + + assert usage.func.cls == CourseVideoUsageViewSet + def test_operation_ids(self): document = production_document() + assert document["paths"][USAGE_PATH]["get"]["operationId"] == "v2_courses_video_usages_retrieve" assert document["paths"][LEGACY_USAGE_PATH]["get"]["operationId"] == "v1_videos_usage_retrieve" assert document["paths"][LEGACY_DOWNLOAD_PATH]["put"]["operationId"] == "v1_videos_download_update" + def test_no_authoring_operation_is_given_the_id_of_another(self): + # Ids are derived from the path after its API prefix, so an authoring + # path can be given the id of a contentstore one. The generator then + # renames one of the two with a numeric suffix, possibly the existing + # operation, so a collision is checked on the ids as first assigned. + generator = SchemaGenerator() + hooks = [*schema_settings(PRODUCTION_SETTINGS)["POSTPROCESSING_HOOKS"], RECORDING_HOOK] + generate_document(postprocessing_hooks=hooks, generator=generator) + holders = defaultdict(list) + for operation_id, path, method in generator.assigned_operation_ids: + holders[operation_id].append((path, method)) + + shared = { + operation_id: places + for operation_id, places in holders.items() + if len(places) > 1 and any(path.startswith("/api/authoring/") for path, __ in places) + } + assert shared == {} + assert sorted((path, method) for __, path, method in generator.assigned_operation_ids) == sorted( + (path, method) for path, method, __ in operations(production_document()) + ) + + # Published operations nothing supersedes: the neighbours of the + # superseded one, other Studio resources, and the new address. Resources + # superseded in their own right are not listed. + UNMARKED_OPERATIONS = ( + ("/api/contentstore/v1/videos/{course_id}", "get"), + (LEGACY_DOWNLOAD_PATH, "put"), + ("/api/contentstore/v0/videos/encodings/{course_id}", "get"), + ("/api/contentstore/v0/videos/features", "get"), + ("/api/contentstore/v0/videos/images/{course_id}/{edx_video_id}", "post"), + ("/api/contentstore/v2/home/courses", "get"), + ("/api/contentstore/v2/downstreams/{usage_key_string}", "get"), + (USAGE_PATH, "get"), + ) + @staticmethod def marks(document, path, method): operation = document["paths"][path][method] return operation.get("deprecated"), operation.get("x-internal") - def test_no_operation_is_flagged_before_its_successor_exists(self): + def test_the_legacy_usage_operation_is_deprecated(self): document = production_document() - flagged = [ - (path, method) - for path, method, __ in operations(document) - if self.marks(document, path, method) != (None, None) - ] - assert flagged == [] + assert self.marks(document, LEGACY_USAGE_PATH, "get") == (True, None) + + def test_other_operations_are_left_unmarked(self): + document = production_document() + + assert {key: self.marks(document, *key) for key in self.UNMARKED_OPERATIONS} == { + key: (None, None) for key in self.UNMARKED_OPERATIONS + } + + # The document declares no security schemes for the platform's default + # authentication, so the refusal text is where a client learns what to send. + @ddt.data( + (USAGE_PATH, "get", ( + "The request carries no accepted credentials. Send a JSON Web Token in the Authorization " + "header as 'JWT ' (the OAuth2 access token endpoint issues one when the token request " + "sets token_type=jwt), or the session cookie of a signed-in Studio user whose account is " + "active. OAuth2 access tokens sent as 'Bearer ' are not accepted." + )), + ) + @ddt.unpack + def test_the_accepted_credentials_are_named(self, path, method, description): + operation = production_document()["paths"][path][method] + + assert operation["responses"]["401"]["description"] == description + + @ddt.data( + (USAGE_PATH, "get", "403", "The requester does not have Studio access to the course."), + ) + @ddt.unpack + def test_each_refusal_names_its_reasons(self, path, method, code, description): + operation = production_document()["paths"][path][method] + + assert operation["responses"][code]["description"] == description + + def test_the_new_operation_is_tagged_for_the_sdk(self): + document = production_document() + + assert document["paths"][USAGE_PATH]["get"]["tags"] == ["openedx-platform-sdk"] + + def component(self, document, reference): + return document["components"]["schemas"][reference["$ref"].rsplit("/", 1)[-1]] + + def test_the_usage_response_is_declared(self): + document = production_document() + response = document["paths"][USAGE_PATH]["get"]["responses"]["200"] + + body = self.component(document, response["content"]["application/json"]["schema"]) + entry = self.component(document, body["properties"]["usage_locations"]["items"]) + assert body["required"] == ["usage_locations"] + assert sorted(entry["properties"]) == ["display_location", "url"] + fields = VideoUsageLocationSerializer().fields + assert {name: entry["properties"][name]["description"] for name in fields} == { + name: field.help_text for name, field in fields.items() + } + + @ddt.data( + (USAGE_PATH, "get", ["401", "403", "404"]), + ) + @ddt.unpack + def test_every_error_response_is_the_error_envelope(self, path, method, codes): + responses = production_document()["paths"][path][method]["responses"] + + assert sorted(code for code in responses if code != "200") == codes + for code in codes: + assert responses[code]["content"]["application/json"]["schema"] == { + "$ref": "#/components/schemas/ErrorResponse" + } def test_enum_components_survive_the_registered_post_processing(self): without_enum_hook = generate_document(postprocessing_hooks=[MIGRATED_PATHS_HOOK]) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_usages.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_usages.py new file mode 100644 index 000000000000..1d6714ba88d1 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_usages.py @@ -0,0 +1,844 @@ +""" +Unit tests for the course video usages API. +""" +import json +from unittest.mock import patch + +import ddt +import pytest +from crum.signals import current_user_getter, current_user_setter +from django.core.signals import got_request_exception, request_finished, request_started +from django.db import connection +from django.db.models.signals import post_init, pre_init +from django.dispatch import Signal +from django.test.utils import CaptureQueriesContext +from django.urls import Resolver404, resolve, reverse +from edx_rest_framework_extensions.auth.jwt.tests.utils import generate_jwt +from edx_rest_framework_extensions.testing import assert_error_envelope +from openedx_authz.models.engine import PolicyCacheControl +from rest_framework import status +from rest_framework.test import APIClient + +from cms.djangoapps.contentstore.rest_api.v1.views.unknown_route import UnknownRouteView +from cms.djangoapps.contentstore.rest_api.v2.views.video_usages import CourseVideoUsageViewSet +from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from common.djangoapps.student.roles import ( + CourseInstructorRole, + CourseLimitedStaffRole, + CourseStaffRole, +) +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.oauth_dispatch.tests.factories import AccessTokenFactory, ApplicationFactory +from xmodule.modulestore.django import SwitchedSignal +from xmodule.modulestore.tests.factories import BlockFactory, CourseFactory + +DETAIL_URL_NAME = "authoring_v2:course_video_usage_detail" +LEGACY_URL_NAME = "cms.djangoapps.contentstore:v1:video_usage" + +MISSING_COURSE_KEY = "course-v1:Org+Missing+Run" +CCX_COURSE_KEY = "ccx-v1:edX+DemoX+Demo_Course+ccx@1" + +# Keys the course_key path converter accepts that carry only a content version, +# with no organization, course or run. +VERSION_ONLY_COURSE_KEYS = ( + "course-v1:version@" + "a" * 24, + "course-v1:branch@published-branch+version@" + "a" * 24, + "ccx-v1:version@" + "a" * 24 + "+ccx@1", + "library-v1:version@" + "a" * 24, +) + +# Fields that legitimately differ per request and are ignored when diffing. +VOLATILE_FIELDS = {"instance"} + +# Every way a successful body of this version may differ from the legacy one, +# as (json_path, reason). The usage body is carried over unchanged, so there are +# none; the parity tests fail on any difference at all. +INTENTIONAL_DIFFERENCES = [] + +# The members of an error body at each address. The legacy address answers with +# its own error format, this version with the standard envelope. +LEGACY_ERROR_KEYS = { + 401: {"developer_message"}, + 403: {"developer_message"}, + 404: {"developer_message", "error_code"}, +} +ENVELOPE_KEYS = {"type", "title", "status", "detail", "instance"} + +# Signals sent while serving any request, whatever it reads: the request +# starting and finishing, model instances being built, and the current user +# being recorded and looked up. +ROUTINE_SIGNALS = ( + request_started, + request_finished, + pre_init, + post_init, + current_user_setter, + current_user_getter, +) + +# The usage lookup each address calls. +SERVICE_USAGE_LOOKUP = "cms.djangoapps.contentstore.rest_api.v2.video_management_service.get_video_usage_path" +LEGACY_USAGE_LOOKUP = "cms.djangoapps.contentstore.rest_api.v1.views.videos.get_video_usage_path" + + +def _normalize(obj, path=""): + """Sort keys, drop volatile fields, and collect a {json_path: value} map for diffing.""" + flat = {} + if isinstance(obj, dict): + for key in sorted(obj): + if key in VOLATILE_FIELDS: + continue + flat.update(_normalize(obj[key], f"{path}.{key}" if path else key)) + elif isinstance(obj, list): + for index, item in enumerate(obj): + flat.update(_normalize(item, f"{path}[{index}]")) + else: + flat[path] = obj + return flat + + +def _matches(json_path, declared): + return json_path == declared or json_path.startswith((declared + ".", declared + "[")) + + +def _diff(legacy, new): + """Return every JSON path that differs, and those of them not declared intentional.""" + flat_legacy, flat_new = _normalize(legacy), _normalize(new) + changed = { + key for key in set(flat_legacy) | set(flat_new) + if flat_legacy.get(key, "") != flat_new.get(key, "") + } + unexpected = { + key for key in changed + if not any(_matches(key, declared) for declared, __ in INTENTIONAL_DIFFERENCES) + } + return changed, unexpected + + +class VideoUsageTestBase(CourseTestCase): + """ + A course whose content exercises every path of the usage lookup. + + ``single-use`` is placed once, ``triple-use`` three times, ``draft-use`` in a + unit that was never published, and ``orphan-use`` once in a unit and once in + a component that has no parent. ``unused`` is referenced by nothing. + """ + + ENABLED_CACHES = ["default"] + + def setUp(self): + super().setUp() + chapter = BlockFactory.create(parent=self.course, category="chapter", display_name="Week 1") + self.subsection = BlockFactory.create(parent=chapter, category="sequential", display_name="Welcome") + self.placements = { + "single-use": [self.place_video("Unit A", "Clip A", "single-use")], + "triple-use": [ + self.place_video(f"Unit T{number}", f"Clip T{number}", "triple-use") + for number in (1, 2, 3) + ], + "orphan-use": [self.place_video("Unit O", "Clip O", "orphan-use")], + "unused": [], + } + # Publishing a unit publishes its whole subsection, so the unpublished + # unit is added after every published one. + self.placements["draft-use"] = [self.place_video("Draft unit", "Draft clip", "draft-use", publish=False)] + self.orphan = self.store.create_item( + self.user.id, + self.course.id, + "video", + block_id="orphan_clip", + fields={"edx_video_id": "orphan-use", "display_name": "Orphan clip"}, + ) + + def place_video(self, unit_name, video_name, edx_video_id, publish=True): + """Add a unit holding one video component to the subsection; return both.""" + unit = BlockFactory.create( + parent=self.subsection, category="vertical", display_name=unit_name, publish_item=publish, + ) + video = BlockFactory.create( + parent=unit, category="video", display_name=video_name, edx_video_id=edx_video_id, + publish_item=publish, + ) + return unit, video + + def expected_body(self, edx_video_id): + """The usage body the lookup must produce for ``edx_video_id``.""" + return { + "usage_locations": [ + { + "display_location": f"Welcome - {unit.display_name} / {video.display_name}", + "url": f"/container/{unit.location}#{video.location}", + } + for unit, video in self.placements[edx_video_id] + ] + } + + def url(self, edx_video_id, course_key=None): + return reverse(DETAIL_URL_NAME, kwargs={ + "course_key": course_key or self.course.id, + "edx_video_id": edx_video_id, + }) + + def legacy_url(self, edx_video_id, course_key=None): + return reverse(LEGACY_URL_NAME, kwargs={ + "course_id": course_key or self.course.id, + "edx_video_id": edx_video_id, + }) + + def client_for(self, user): + client = APIClient() + if user is not None: + client.force_authenticate(user=user) + return client + + def user_with_role(self, role_class, course_key=None): + user = UserFactory.create() + role_class(course_key or self.course.id).add_users(user) + return user + + +@ddt.ddt +class VideoUsageResponseTest(VideoUsageTestBase): + """The body names every component that plays the video, and nothing else.""" + + @ddt.data("single-use", "triple-use", "draft-use", "orphan-use", "unused") + def test_body_lists_each_placement(self, edx_video_id): + response = self.client_for(self.user).get(self.url(edx_video_id)) + + assert response.status_code == status.HTTP_200_OK + assert response.json() == self.expected_body(edx_video_id) + + def test_a_video_used_three_times_is_listed_three_times_in_content_order(self): + response = self.client_for(self.user).get(self.url("triple-use")) + + assert [entry["display_location"] for entry in response.json()["usage_locations"]] == [ + "Welcome - Unit T1 / Clip T1", + "Welcome - Unit T2 / Clip T2", + "Welcome - Unit T3 / Clip T3", + ] + + def test_a_component_in_an_unpublished_unit_is_listed(self): + unit, __ = self.placements["draft-use"][0] + assert not self.store.has_published_version(unit) + + response = self.client_for(self.user).get(self.url("draft-use")) + + assert response.json()["usage_locations"] == [{ + "display_location": "Welcome - Draft unit / Draft clip", + "url": f"/container/{unit.location}#{self.placements['draft-use'][0][1].location}", + }] + + def test_a_component_without_a_parent_is_skipped(self): + referencing = [ + block.location.block_id + for block in self.store.get_items(self.course.id, qualifiers={"category": "video"}) + if block.edx_video_id == "orphan-use" + ] + assert sorted(referencing) == sorted(["orphan_clip", self.placements["orphan-use"][0][1].location.block_id]) + + response = self.client_for(self.user).get(self.url("orphan-use")) + + assert response.json() == self.expected_body("orphan-use") + assert len(response.json()["usage_locations"]) == 1 + + @ddt.data("url", "legacy_url") + def test_names_the_content_does_not_set_appear_as_their_field_defaults(self, address): + chapter = BlockFactory.create(parent=self.course, category="chapter", display_name="Week 2") + subsection = BlockFactory.create(parent=chapter, category="sequential", display_name=None) + unit = BlockFactory.create(parent=subsection, category="vertical", display_name=None) + BlockFactory.create(parent=unit, category="video", display_name=None, edx_video_id="unnamed-use") + + response = self.client_for(self.user).get(getattr(self, address)("unnamed-use")) + + assert [entry["display_location"] for entry in response.json()["usage_locations"]] == [ + "None - None / Video", + ] + + @ddt.data("url", "legacy_url") + def test_a_unit_in_no_subsection_leaves_the_subsection_part_empty(self, address): + unit = self.store.create_item( + self.user.id, self.course.id, "vertical", block_id="loose_unit", fields={"display_name": "Loose unit"}, + ) + BlockFactory.create(parent=unit, category="video", display_name="Loose clip", edx_video_id="loose-use") + + response = self.client_for(self.user).get(getattr(self, address)("loose-use")) + + assert [entry["display_location"] for entry in response.json()["usage_locations"]] == [ + " - Loose unit / Loose clip", + ] + + def test_an_unreferenced_video_answers_with_an_empty_list(self): + response = self.client_for(self.user).get(self.url("unused")) + + assert response.status_code == status.HTTP_200_OK + assert response.json() == {"usage_locations": []} + + def test_head_answers_like_get_without_a_body(self): + response = self.client_for(self.user).head(self.url("single-use")) + + assert response.status_code == status.HTTP_200_OK + assert response.content == b"" + + +@ddt.ddt +class VideoUsageParityTest(VideoUsageTestBase): + """For the same course, video and caller, both addresses answer alike.""" + + def pair(self, edx_video_id, user, course_key=None, method="get"): + client = self.client_for(user) + call = getattr(client, method) + return ( + call(self.legacy_url(edx_video_id, course_key)), + call(self.url(edx_video_id, course_key)), + ) + + def assert_parity(self, legacy, new, expected_status): + """Assert both answered ``expected_status`` and differ only where declared.""" + assert legacy.status_code == expected_status, legacy.content + assert new.status_code == expected_status, new.content + legacy_json = json.loads(legacy.content or b"null") + new_json = json.loads(new.content or b"null") + changed, unexpected = _diff(legacy_json, new_json) + assert not unexpected, f"unexpected differences: {sorted(unexpected)}\nlegacy={legacy_json}\nnew={new_json}" + for declared, reason in INTENTIONAL_DIFFERENCES: + assert any(_matches(key, declared) for key in changed), ( + f"declared difference did not occur: {declared} ({reason})" + ) + + @ddt.data("single-use", "triple-use", "draft-use", "orphan-use", "unused") + def test_success_bodies_are_identical(self, edx_video_id): + legacy, new = self.pair(edx_video_id, self.user) + + self.assert_parity(legacy, new, status.HTTP_200_OK) + assert new.json() == self.expected_body(edx_video_id) + assert new.content == legacy.content + + def test_course_staff_sees_the_same_body_at_both_addresses(self): + legacy, new = self.pair("triple-use", self.user_with_role(CourseStaffRole)) + + self.assert_parity(legacy, new, status.HTTP_200_OK) + assert new.json() == self.expected_body("triple-use") + + def test_head_answers_alike(self): + legacy, new = self.pair("single-use", self.user, method="head") + + assert (legacy.status_code, legacy.content) == (status.HTTP_200_OK, b"") + assert (new.status_code, new.content) == (status.HTTP_200_OK, b"") + + def assert_error_keys(self, legacy, new, expected_status): + assert legacy.status_code == new.status_code == expected_status + assert set(legacy.json()) == LEGACY_ERROR_KEYS[expected_status] + assert set(new.json()) == ENVELOPE_KEYS + + def test_unauthenticated_error_bodies(self): + legacy, new = self.pair("single-use", None) + + self.assert_error_keys(legacy, new, status.HTTP_401_UNAUTHORIZED) + assert_error_envelope(new, expected_status=401, expected_type_slug="authn") + + def test_forbidden_error_bodies(self): + legacy, new = self.pair("single-use", UserFactory.create()) + + self.assert_error_keys(legacy, new, status.HTTP_403_FORBIDDEN) + assert_error_envelope(new, expected_status=403, expected_type_slug="authz") + + def test_missing_course_error_bodies(self): + legacy, new = self.pair("single-use", self.user, course_key=MISSING_COURSE_KEY) + + self.assert_error_keys(legacy, new, status.HTTP_404_NOT_FOUND) + assert legacy.json()["error_code"] == "course_does_not_exist" + assert_error_envelope(new, expected_status=404, expected_type_slug="not-found") + + +@ddt.ddt +class VideoUsageAuthorizationTest(VideoUsageTestBase): + """Studio read access to the course is required; everyone else is refused.""" + + def assert_allowed(self, user): + response = self.client_for(user).get(self.url("triple-use")) + assert response.status_code == status.HTTP_200_OK + assert response.json() == self.expected_body("triple-use") + + def test_global_staff_is_allowed(self): + self.assert_allowed(self.user) + + def test_course_staff_is_allowed(self): + self.assert_allowed(self.user_with_role(CourseStaffRole)) + + def test_course_instructor_is_allowed(self): + self.assert_allowed(self.user_with_role(CourseInstructorRole)) + + @ddt.data("get", "head") + def test_anonymous_is_refused(self, method): + response = getattr(self.client_for(None), method)(self.url("single-use")) + + assert response.status_code == status.HTTP_401_UNAUTHORIZED + assert response["WWW-Authenticate"] == 'JWT realm="api"' + if method == "get": + assert_error_envelope(response, expected_status=401, expected_type_slug="authn") + + @ddt.data("get", "head") + def test_user_without_a_role_is_refused(self, method): + response = getattr(self.client_for(UserFactory.create()), method)(self.url("single-use")) + + assert response.status_code == status.HTTP_403_FORBIDDEN + if method == "get": + assert_error_envelope(response, expected_status=403, expected_type_slug="authz") + + def test_limited_staff_is_refused(self): + response = self.client_for(self.user_with_role(CourseLimitedStaffRole)).get(self.url("single-use")) + + assert_error_envelope(response, expected_status=403, expected_type_slug="authz") + + def test_staff_of_another_course_is_refused(self): + other_course = CourseFactory.create() + author = self.user_with_role(CourseStaffRole, course_key=other_course.id) + + response = self.client_for(author).get(self.url("single-use")) + + assert_error_envelope(response, expected_status=403, expected_type_slug="authz") + + def test_ccx_course_is_refused_even_to_global_staff(self): + response = self.client_for(self.user).get(self.url("single-use", course_key=CCX_COURSE_KEY)) + + assert_error_envelope(response, expected_status=403, expected_type_slug="authz") + + def test_missing_course_is_refused_to_a_caller_without_access(self): + response = self.client_for(UserFactory.create()).get(self.url("single-use", course_key=MISSING_COURSE_KEY)) + + assert_error_envelope(response, expected_status=403, expected_type_slug="authz") + + def test_missing_course_is_reported_to_global_staff(self): + response = self.client_for(self.user).get(self.url("single-use", course_key=MISSING_COURSE_KEY)) + + envelope = assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + assert envelope["detail"] == "The course does not exist." + + @ddt.data(*VERSION_ONLY_COURSE_KEYS) + def test_a_key_carrying_only_a_version_is_not_found_by_global_staff(self, key): + response = self.client_for(self.user).get(self.url("single-use", course_key=key)) + + envelope = assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + assert envelope["detail"] == "Not found." + + @ddt.data(*VERSION_ONLY_COURSE_KEYS) + def test_a_key_carrying_only_a_version_is_not_found_by_a_caller_without_a_role(self, key): + response = self.client_for(UserFactory.create()).get(self.url("single-use", course_key=key)) + + envelope = assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + assert envelope["detail"] == "Not found." + + @ddt.data(*VERSION_ONLY_COURSE_KEYS) + def test_a_key_carrying_only_a_version_still_needs_authentication(self, key): + response = self.client_for(None).get(self.url("single-use", course_key=key)) + + assert_error_envelope(response, expected_status=401, expected_type_slug="authn") + + def test_the_legacy_address_answers_404_for_a_version_only_key(self): + # The legacy pattern needs a separator inside the key, so the address + # falls through to the course videos listing, which cannot parse the + # rest of the path as a course key. + address = f"/api/contentstore/v1/videos/{VERSION_ONLY_COURSE_KEYS[0]}/single-use/usage" + + response = self.client_for(self.user).get(address) + + assert resolve(address).func.view_class.__name__ == "CourseVideosView" + assert response.status_code == status.HTTP_404_NOT_FOUND + assert response.json()["error_code"] == "invalid_course_key" + + def test_the_legacy_address_reports_a_missing_course_to_anyone(self): + response = self.client_for(UserFactory.create()).get( + self.legacy_url("single-use", course_key=MISSING_COURSE_KEY) + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + assert response.json()["error_code"] == "course_does_not_exist" + + def test_the_view_declares_its_checks_in_order(self): + # The JWT middleware appends its own restricted-application check to + # views that accept JWTs, so only the declared head of the list is fixed. + assert [klass.__name__ for klass in CourseVideoUsageViewSet.permission_classes[:3]] == [ + "IsAuthenticated", "HasFullCourseKey", "HasStudioReadAccess", + ] + + +class VideoUsageUnexpectedFailureTest(VideoUsageTestBase): + """ + An unexpected failure behind the lookup, at both addresses. + + The new address answers with the generic 500 body from inside the view, so + the exception never reaches Django: ``got_request_exception`` is not sent, + and the only log record is Django's one-line 500 notice, without the + traceback. The legacy address lets the exception reach Django, which sends + the signal and logs the traceback. Both sides are pinned, so a change in how + the error handling reports failures shows up here. + """ + + def request_while_failing(self, address, target): + """GET ``address`` as global staff while ``target`` raises; return the response, signals and log records.""" + signals = [] + + def receiver(sender, **kwargs): + signals.append(sender) + + got_request_exception.connect(receiver) + self.addCleanup(got_request_exception.disconnect, receiver) + client = APIClient(raise_request_exception=False) + client.force_authenticate(user=self.user) + with patch(target, side_effect=RuntimeError("content store unreachable at mongo-7")): + with self.assertLogs(level="ERROR") as logs: + response = client.get(address) + return response, signals, logs.records + + def test_the_failure_is_answered_with_the_generic_body(self): + response, __, __ = self.request_while_failing(self.url("single-use"), SERVICE_USAGE_LOOKUP) + + envelope = assert_error_envelope(response, expected_status=500, expected_type_slug="internal") + assert envelope["detail"] == "An unexpected error occurred. Please try again later." + assert "mongo-7" not in response.content.decode() + + def test_the_failure_does_not_reach_django(self): + address = self.url("single-use") + + __, signals, records = self.request_while_failing(address, SERVICE_USAGE_LOOKUP) + + assert not signals + assert [(record.name, record.getMessage(), record.exc_info) for record in records] == [ + ("django.request", f"Internal Server Error: {address}", None), + ] + + def test_the_legacy_address_hands_the_failure_to_django(self): + __, signals, records = self.request_while_failing(self.legacy_url("single-use"), LEGACY_USAGE_LOOKUP) + + assert len(signals) == 1 + assert "django.request" in { + record.name for record in records if record.exc_info and record.exc_info[0] is RuntimeError + } + + +class VideoUsageAuthenticationTest(VideoUsageTestBase): + """The endpoint uses the platform's default authentication.""" + + def test_session_caller_succeeds(self): + client = APIClient() + assert client.login(username=self.user.username, password=self.user_password) + + response = client.get(self.url("single-use")) + + assert response.status_code == status.HTTP_200_OK + assert response.json() == self.expected_body("single-use") + + def test_jwt_caller_succeeds(self): + response = APIClient().get(self.url("single-use"), HTTP_AUTHORIZATION=f"JWT {generate_jwt(self.user)}") + + assert response.status_code == status.HTTP_200_OK + assert response.json() == self.expected_body("single-use") + + def bearer_header(self, user): + token = AccessTokenFactory(user=user, application=ApplicationFactory()).token + return f"Bearer {token}" + + def test_bearer_token_is_refused(self): + response = APIClient().get(self.url("single-use"), HTTP_AUTHORIZATION=self.bearer_header(self.user)) + + assert_error_envelope(response, expected_status=401, expected_type_slug="authn") + + def test_the_legacy_address_still_accepts_a_bearer_token(self): + response = APIClient().get(self.legacy_url("single-use"), HTTP_AUTHORIZATION=self.bearer_header(self.user)) + + assert response.status_code == status.HTTP_200_OK + + def deactivated_global_staff(self): + """Log a global staff member in, then deactivate the account behind the live session.""" + password = "password-12345" + staff = UserFactory.create(is_staff=True, password=password) + session_client = APIClient() + assert session_client.login(username=staff.username, password=password) + staff.is_active = False + staff.save() + return staff, session_client + + def test_inactive_global_staff_is_refused_over_a_session(self): + __, session_client = self.deactivated_global_staff() + + response = session_client.get(self.url("single-use")) + + assert_error_envelope(response, expected_status=401, expected_type_slug="authn") + + def test_inactive_global_staff_still_reaches_the_legacy_address_over_a_session(self): + __, session_client = self.deactivated_global_staff() + + response = session_client.get(self.legacy_url("single-use")) + + assert response.status_code == status.HTTP_200_OK + + def test_inactive_global_staff_is_admitted_over_jwt_at_both_addresses(self): + staff, __ = self.deactivated_global_staff() + header = f"JWT {generate_jwt(staff)}" + + new = APIClient().get(self.url("single-use"), HTTP_AUTHORIZATION=header) + legacy = APIClient().get(self.legacy_url("single-use"), HTTP_AUTHORIZATION=header) + + assert (new.status_code, legacy.status_code) == (status.HTTP_200_OK, status.HTTP_200_OK) + assert new.json() == self.expected_body("single-use") + + def test_the_viewset_does_not_declare_authentication_classes(self): + assert "authentication_classes" not in CourseVideoUsageViewSet.__dict__ + assert [klass.__name__ for klass in CourseVideoUsageViewSet.authentication_classes] == [ + "DefaultJwtAuthentication", "DefaultSessionAuthentication", + ] + + +@ddt.ddt +class VideoUsageReadOnlyTest(VideoUsageTestBase): + """ + Reading usages, at either address, writes nothing to the database and sends + no signal or event beyond the ones every request sends. + """ + + def setUp(self): + super().setUp() + # The authorization engine stores a one-row policy cache marker the + # first time any role is checked against an empty database. That is the + # engine starting up, not this endpoint, so it is created up front. + PolicyCacheControl.get_version() + + def session_client(self, role_class=None): + """Return a client logged in over a session as a new user holding ``role_class``, if any.""" + password = "password-12345" + user = UserFactory.create(password=password) + if role_class: + role_class(self.course.id).add_users(user) + client = APIClient() + assert client.login(username=user.username, password=password) + return client + + def side_effects_during(self, request): + """ + Run ``request``; return its response, every INSERT, UPDATE or DELETE it + issued, and every signal it sent beyond ``ROUTINE_SIGNALS``, as + (signal, sender) pairs. + + Signals are recorded as they are sent, whether or not anything receives + them, and Open edX events are sent as signals, so an event published with + no receiver writing to the database is still seen. + """ + sent = [] + + def recording(send): + def record(signal, *args, **named): + if signal not in ROUTINE_SIGNALS: + sent.append((signal, args[0] if args else named.get("sender"))) + return send(signal, *args, **named) + return record + + with ( + patch.object(Signal, "send", recording(Signal.send)), + patch.object(Signal, "send_robust", recording(Signal.send_robust)), + # The test case switches the content store's signals off, and a + # switched-off signal returns before it reaches Signal.send. + patch.object(SwitchedSignal, "send", recording(SwitchedSignal.send)), + patch.object(SwitchedSignal, "send_robust", recording(SwitchedSignal.send_robust)), + CaptureQueriesContext(connection) as captured, + ): + response = request() + writes = [ + query["sql"] for query in captured.captured_queries + if query["sql"].lstrip().split(" ", 1)[0].upper() in ("INSERT", "UPDATE", "DELETE") + ] + return response, writes, sent + + @ddt.data( + *[ + (edx_video_id, method, address) + for edx_video_id in ("single-use", "triple-use", "draft-use", "orphan-use", "unused") + for method in ("get", "head") + for address in ("url", "legacy_url") + ] + ) + @ddt.unpack + def test_course_staff_over_a_session_writes_nothing(self, edx_video_id, method, address): + client = self.session_client(CourseStaffRole) + + response, writes, signals = self.side_effects_during( + lambda: getattr(client, method)(getattr(self, address)(edx_video_id)) + ) + + assert response.status_code == status.HTTP_200_OK + assert writes == [] + assert not signals + + @ddt.data( + *[(method, address) for method in ("get", "head") for address in ("url", "legacy_url")] + ) + @ddt.unpack + def test_global_staff_writes_nothing(self, method, address): + client = self.client_for(self.user) + + response, writes, signals = self.side_effects_during( + lambda: getattr(client, method)(getattr(self, address)("triple-use")) + ) + + assert response.status_code == status.HTTP_200_OK + assert writes == [] + assert not signals + + @ddt.data("url", "legacy_url") + def test_a_missing_course_is_not_created_by_looking_it_up(self, address): + response, writes, signals = self.side_effects_during( + lambda: self.client_for(self.user).get(getattr(self, address)("single-use", MISSING_COURSE_KEY)) + ) + + assert response.status_code == status.HTTP_404_NOT_FOUND + assert writes == [] + assert not signals + + def test_a_refused_caller_writes_nothing(self): + client = self.session_client() + + response, writes, signals = self.side_effects_during(lambda: client.get(self.url("single-use"))) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert writes == [] + assert not signals + + +class VideoUsageQueryCountTest(VideoUsageTestBase): + """ + The new address costs no more queries than the legacy one for the same caller. + + Each error answered by the new address ends with one extra statement, a + rollback of the request's savepoint, issued by the standard error handling. + """ + + #: Set from the pytest fixture below, which class-based tests cannot request directly. + django_assert_num_queries = None + + @pytest.fixture(autouse=True) + def _assert_num_queries(self, django_assert_num_queries): + self.django_assert_num_queries = django_assert_num_queries + + def measure(self, client, legacy_count, new_count, course_key=None): + """Warm the per-process caches, then count the queries of each address.""" + for __ in range(2): + client.get(self.legacy_url("triple-use", course_key)) + client.get(self.url("triple-use", course_key)) + with self.django_assert_num_queries(legacy_count): + client.get(self.legacy_url("triple-use", course_key)) + with self.django_assert_num_queries(new_count): + client.get(self.url("triple-use", course_key)) + + def test_global_staff(self): + self.measure(self.client_for(self.user), legacy_count=11, new_count=11) + + def test_global_staff_over_a_session(self): + self.measure(self.client, legacy_count=13, new_count=13) + + def test_course_staff(self): + self.measure(self.client_for(self.user_with_role(CourseStaffRole)), legacy_count=11, new_count=11) + + def test_refused_caller(self): + self.measure(self.client_for(UserFactory.create()), legacy_count=8, new_count=7) + + def test_missing_course(self): + self.measure(self.client_for(self.user), legacy_count=8, new_count=9, course_key=MISSING_COURSE_KEY) + + def test_anonymous(self): + self.measure(self.client_for(None), legacy_count=5, new_count=6) + + +@ddt.ddt +class VideoUsageUrlContractTest(VideoUsageTestBase): + """The address of a video's usages, and how unusable addresses are answered.""" + + def test_conforming_address(self): + assert self.url("abc-123") == f"/api/authoring/v2/courses/{self.course.id}/video_usages/abc-123/" + + def test_the_address_resolves_to_the_viewset(self): + match = resolve(self.url("abc-123")) + + assert match.func.cls is CourseVideoUsageViewSet + assert match.kwargs == {"course_key": self.course.id, "edx_video_id": "abc-123"} + + def test_the_legacy_address_is_unchanged(self): + assert self.legacy_url("abc-123") == f"/api/contentstore/v1/videos/{self.course.id}/abc-123/usage" + assert resolve(self.legacy_url("abc-123")).func.view_class.__name__ == "VideoUsageView" + + def assert_json_not_found(self, address): + response = self.client_for(self.user).get(address, HTTP_ACCEPT="application/json") + assert response["Content-Type"] == "application/json" + return assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + + def test_malformed_course_key_is_answered_with_the_error_envelope(self): + self.assert_json_not_found("/api/authoring/v2/courses/not-a-key/video_usages/abc-123/") + + def test_deprecated_course_key_is_answered_with_the_error_envelope(self): + self.assert_json_not_found("/api/authoring/v2/courses/Org/Course/Run/video_usages/abc-123/") + + def test_a_block_key_in_place_of_a_course_key_is_answered_with_the_error_envelope(self): + self.assert_json_not_found("/api/authoring/v2/courses/i4x://Org/Course/video/abc/video_usages/abc-123/") + + def test_an_unusable_course_key_is_claimed_by_the_fallback_route(self): + address = "/api/authoring/v2/courses/Org/Course/Run/video_usages/abc-123/" + + assert resolve(address).func.cls is UnknownRouteView + assert reverse( + "authoring_v2:course_video_usage_detail_unmatched", + kwargs={"course_key": "Org/Course/Run", "subpath": "abc-123"}, + ) == address + assert reverse( + "authoring_v2:course_video_usage_list_unmatched", kwargs={"course_key": "Org/Course/Run"}, + ) == "/api/authoring/v2/courses/Org/Course/Run/video_usages/" + + def test_the_fallback_needs_no_authentication(self): + response = self.client_for(None).get("/api/authoring/v2/courses/not-a-key/video_usages/abc-123/") + + assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + + def test_the_legacy_address_still_accepts_a_deprecated_course_key(self): + match = resolve("/api/contentstore/v1/videos/Org/Course/Run/abc-123/usage") + + assert match.func.view_class.__name__ == "VideoUsageView" + assert match.kwargs == {"course_id": "Org/Course/Run", "edx_video_id": "abc-123"} + + @ddt.data( + "video_usages/", + "video_usages//", + "video_usages/abc-123/extra/", + "video_usages/abc-123/extra/more/", + ) + def test_other_addresses_under_the_collection_are_answered_with_the_error_envelope(self, rest): + address = f"/api/authoring/v2/courses/{self.course.id}/{rest}" + + assert resolve(address).func.cls is UnknownRouteView + self.assert_json_not_found(address) + + @ddt.data("video_usages", "video_usages/abc-123/extra") + def test_an_address_under_the_collection_without_the_trailing_slash_is_redirected(self, rest): + address = f"/api/authoring/v2/courses/{self.course.id}/{rest}" + + response = self.client_for(self.user).get(address, HTTP_ACCEPT="application/json") + + assert response.status_code == status.HTTP_301_MOVED_PERMANENTLY + assert response["Location"] == f"{address}/" + + def test_the_address_requires_the_trailing_slash(self): + with pytest.raises(Resolver404): + resolve(self.url("abc-123").rstrip("/")) + + def test_the_trailing_slash_is_appended_by_redirect(self): + response = self.client_for(self.user).get(self.url("abc-123").rstrip("/")) + + assert response.status_code == status.HTTP_301_MOVED_PERMANENTLY + assert response["Location"] == self.url("abc-123") + + def test_an_unsupported_method_is_refused(self): + response = self.client_for(self.user).post(self.url("single-use"), {}, format="json") + + assert_error_envelope(response, expected_status=405) + assert response["Allow"] == "GET, HEAD, OPTIONS" + + def test_an_unsatisfiable_accept_header_is_refused(self): + response = self.client_for(self.user).get(self.url("single-use"), HTTP_ACCEPT="application/zip") + + assert_error_envelope(response, expected_status=406) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/video_schema.py b/cms/djangoapps/contentstore/rest_api/v2/views/video_schema.py new file mode 100644 index 000000000000..f7583fc8b121 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/video_schema.py @@ -0,0 +1,47 @@ +"""Schema parameters and responses shared by the API views of a course's videos.""" + +from drf_spectacular.utils import OpenApiParameter, OpenApiResponse +from edx_rest_framework_extensions.errors import ErrorResponseSerializer + +COURSE_KEY_PARAMETER = OpenApiParameter( + name="course_key", + description="Key of the course, for example course-v1:edX+DemoX+Demo_Course.", + required=True, + type=str, + location=OpenApiParameter.PATH, +) + +COURSE_NOT_FOUND_RESPONSE = OpenApiResponse( + response=ErrorResponseSerializer, + description="The course does not exist, or the course key is malformed.", +) + + +def unauthenticated_response(requires_csrf_token): + """ + Return the 401 response, naming the credentials the platform accepts. + + ``requires_csrf_token`` is true for an operation whose session callers must + also send the CSRF token, as for any method other than GET, HEAD, OPTIONS + and TRACE. + """ + session = "the session cookie of a signed-in Studio user whose account is active" + if requires_csrf_token: + session += ", together with the CSRF token in the X-CSRFToken header" + return OpenApiResponse( + response=ErrorResponseSerializer, + description=( + "The request carries no accepted credentials. Send a JSON Web Token in the Authorization " + "header as 'JWT ' (the OAuth2 access token endpoint issues one when the token request " + f"sets token_type=jwt), or {session}. OAuth2 access tokens sent as 'Bearer ' are not " + "accepted." + ), + ) + + +def forbidden_response(requires_csrf_token): + """Return the 403 response; ``requires_csrf_token`` as for ``unauthenticated_response``.""" + description = "The requester does not have Studio access to the course" + if requires_csrf_token: + description += ", or a request made with a session did not send the CSRF token" + return OpenApiResponse(response=ErrorResponseSerializer, description=f"{description}.") diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/video_usages.py b/cms/djangoapps/contentstore/rest_api/v2/views/video_usages.py new file mode 100644 index 000000000000..f7617dc61041 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/video_usages.py @@ -0,0 +1,97 @@ +"""API Views for where a video is used in a course.""" + +from drf_spectacular.utils import OpenApiExample, OpenApiParameter, OpenApiResponse, extend_schema +from edx_rest_framework_extensions.mixins import StandardizedErrorMixin +from rest_framework import viewsets +from rest_framework.permissions import IsAuthenticated +from rest_framework.response import Response + +from cms.djangoapps.contentstore.rest_api.v2.serializers.video_usages import CourseVideoUsageSerializer +from cms.djangoapps.contentstore.rest_api.v2.video_management_service import get_course_video_usages +from cms.djangoapps.contentstore.rest_api.v2.views.video_schema import ( + COURSE_KEY_PARAMETER, + COURSE_NOT_FOUND_RESPONSE, + forbidden_response, + unauthenticated_response, +) +from cms.djangoapps.contentstore.views.permissions import HasFullCourseKey, HasStudioReadAccess + +_EDX_VIDEO_ID_PARAMETER = OpenApiParameter( + name="edx_video_id", + description="Identifier of the video in the video store, as set on the video components that play it.", + required=True, + type=str, + location=OpenApiParameter.PATH, +) + +_USAGE_EXAMPLE = OpenApiExample( + "A video placed in two units", + value={ + "usage_locations": [ + { + "display_location": "Week 1 - Welcome / Introduction video", + "url": ( + "/container/block-v1:edX+DemoX+Demo_Course+type@vertical+block@welcome" + "#block-v1:edX+DemoX+Demo_Course+type@video+block@intro" + ), + }, + { + "display_location": "Week 3 - Recap / Introduction video", + "url": ( + "/container/block-v1:edX+DemoX+Demo_Course+type@vertical+block@recap" + "#block-v1:edX+DemoX+Demo_Course+type@video+block@intro_again" + ), + }, + ] + }, + response_only=True, +) + + +@extend_schema(tags=["openedx-platform-sdk"]) +class CourseVideoUsageViewSet(StandardizedErrorMixin, viewsets.ViewSet): + """ + Where one video of a course is used. + + Answers with every video component in the course that references the video, + including components in unpublished units. Callers need Studio read access + to the course; a caller without it is refused before the course is looked + up, so the refusal does not reveal whether the course exists. + + The usages come from walking the course's video components in the content + store, not from a database query, so this is a plain ViewSet with no + queryset. An identifier that no component references answers with an empty + list rather than 404, because components may reference videos that the + video store does not hold. + """ + + permission_classes = (IsAuthenticated, HasFullCourseKey, HasStudioReadAccess) + serializer_class = CourseVideoUsageSerializer + + def get_serializer(self, *args, **kwargs): + """Instantiate and return the configured serializer class.""" + return self.serializer_class(*args, **kwargs) + + @extend_schema( + summary="Get where a video is used in a course", + description=( + "Returns one entry for every video component in the course that plays the video, " + "including components in units that are not published yet. Each entry names the " + "subsection, unit and component, and gives the Studio address of the unit." + ), + parameters=[COURSE_KEY_PARAMETER, _EDX_VIDEO_ID_PARAMETER], + responses={ + 200: OpenApiResponse( + response=CourseVideoUsageSerializer, + description="The places the video is used; an empty list when it is used nowhere.", + examples=[_USAGE_EXAMPLE], + ), + 401: unauthenticated_response(requires_csrf_token=False), + 403: forbidden_response(requires_csrf_token=False), + 404: COURSE_NOT_FOUND_RESPONSE, + }, + ) + def retrieve(self, request, course_key, edx_video_id): + """Return the places in the course where the video is used.""" + usages = get_course_video_usages(course_key, edx_video_id) + return Response(self.get_serializer(usages).data) diff --git a/cms/djangoapps/contentstore/views/permissions.py b/cms/djangoapps/contentstore/views/permissions.py index 6d63c7a124f4..b3fa8e0eae0a 100644 --- a/cms/djangoapps/contentstore/views/permissions.py +++ b/cms/djangoapps/contentstore/views/permissions.py @@ -2,9 +2,10 @@ Custom permissions for the content store views. """ +from rest_framework.exceptions import NotFound from rest_framework.permissions import BasePermission -from common.djangoapps.student.auth import has_studio_write_access +from common.djangoapps.student.auth import has_studio_read_access, has_studio_write_access from openedx.core.lib.api.view_utils import validate_course_key @@ -20,3 +21,36 @@ def has_permission(self, request, view): course_key_string = view.kwargs.get("course_key_string") course_key = validate_course_key(course_key_string) return has_studio_write_access(request.user, course_key) + + +class HasStudioReadAccess(BasePermission): + """ + Allow callers with Studio read access to the course in the path. + + Reads the parsed ``course_key`` URL argument. + """ + + def has_permission(self, request, view): + course_key = view.kwargs.get("course_key") + if course_key is None: + return False + return has_studio_read_access(request.user, course_key) + + +class HasFullCourseKey(BasePermission): + """ + Answer 404 for a course key in the path that carries only a content version. + + Reads the parsed ``course_key`` URL argument. A key such as + ``course-v1:version@`` parses, but names no organization, course or + run: no course can be found by it, and Studio's role checks raise an error + on it instead of refusing it. The request is answered as an address that + holds nothing, as for a malformed key. List this class after the + authentication check and before any class that checks roles on the course. + """ + + def has_permission(self, request, view): + course_key = view.kwargs.get("course_key") + if course_key is not None and course_key.org is None: + raise NotFound() + return True diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index 1c004d442b91..22ae63b60c24 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -4,7 +4,10 @@ # Path prefixes of operations superseded by a newer version, kept for their # deprecation window. -SUPERSEDED_PATH_PREFIXES = () +SUPERSEDED_PATH_PREFIXES = ( + # → /api/authoring/v2/courses/{course_key}/video_usages/{edx_video_id}/ + "/api/contentstore/v1/videos/{course_id}/{edx_video_id}/usage", +) def cms_api_filter(endpoints): diff --git a/cms/urls.py b/cms/urls.py index f8140f15422f..5f8864647abf 100644 --- a/cms/urls.py +++ b/cms/urls.py @@ -360,6 +360,10 @@ path('api/contentstore/', include('cms.djangoapps.contentstore.rest_api.urls')) ] +urlpatterns += [ + path('api/authoring/v2/', include('cms.djangoapps.contentstore.rest_api.v2.authoring_urls')), +] + # Content tagging urlpatterns += [ path('api/content_tagging/', include(('openedx.core.djangoapps.content_tagging.urls', 'content_tagging'))), From f20db7b6e40ce6930d01877bc492fecf43e6f144 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Mon, 5 Oct 2026 00:46:16 +0500 Subject: [PATCH 3/7] feat: add a v2 video archives endpoint to the authoring API POST /api/authoring/v2/courses/{course_key}/video_archives/ streams a zip of chosen videos of a course. It replaces PUT /api/contentstore/v1/videos/{course_id}/download, whose verb path, unvalidated body (a malformed request got a 500) and error format cannot change in place without breaking its callers. The request body is validated by a serializer and accepted as JSON only. Every requested URL must still be one of the course's own encoded-video URLs before anything is fetched, through the existing helper, and the existing per-user throttle is shared with v1, so both addresses draw on one budget. The v1 operation is marked deprecated in the schema; its view is unchanged. Part of #39071. --- .../rest_api/v2/authoring_urls.py | 35 +- .../rest_api/v2/serializers/video_archives.py | 35 + .../rest_api/v2/video_management_service.py | 15 +- .../v2/views/tests/test_video_archives.py | 1024 +++++++++++++++++ .../tests/test_video_management_contract.py | 113 +- .../rest_api/v2/views/video_archives.py | 127 ++ cms/lib/spectacular.py | 2 + 7 files changed, 1315 insertions(+), 36 deletions(-) create mode 100644 cms/djangoapps/contentstore/rest_api/v2/serializers/video_archives.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_archives.py create mode 100644 cms/djangoapps/contentstore/rest_api/v2/views/video_archives.py diff --git a/cms/djangoapps/contentstore/rest_api/v2/authoring_urls.py b/cms/djangoapps/contentstore/rest_api/v2/authoring_urls.py index 82cd1802dc87..0b448652335c 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/authoring_urls.py +++ b/cms/djangoapps/contentstore/rest_api/v2/authoring_urls.py @@ -3,6 +3,7 @@ from django.urls import path, re_path from cms.djangoapps.contentstore.rest_api.v1.views.unknown_route import UnknownRouteView +from cms.djangoapps.contentstore.rest_api.v2.views.video_archives import CourseVideoArchiveView from cms.djangoapps.contentstore.rest_api.v2.views.video_usages import CourseVideoUsageViewSet app_name = "authoring_v2" @@ -13,15 +14,21 @@ CourseVideoUsageViewSet.as_view({"get": "retrieve"}), name="course_video_usage_detail", ), - # Every other address under this collection holds nothing: a course key - # the route above refuses (malformed, or in the deprecated slash-separated - # form), the usage collection without a video id, an empty video id, or a - # path below a usage. They answer with the API error body rather than the - # site's HTML error page. They match addresses under this collection and - # nothing else, so a resource added under courses/ is never shadowed, and - # only slash-terminated ones, so the redirect to the slash-terminated form - # still happens first. A path converter cannot match an empty segment, - # hence the pattern for the addresses below a usage. + path( + "courses//video_archives/", + CourseVideoArchiveView.as_view(), + name="course_video_archive_list", + ), + # Every other address under these two collections holds nothing: a course + # key the routes above refuse (malformed, or in the deprecated + # slash-separated form), the usage collection without a video id, an empty + # video id, or a path below a usage or below the archives. They answer with + # the API error body rather than the site's HTML error page. They match + # addresses under these two collections and nothing else, so a resource + # added under courses/ is never shadowed, and only slash-terminated ones, so + # the redirect to the slash-terminated form still happens first. A path + # converter cannot match an empty segment, hence the patterns for the + # addresses below each collection. path( "courses//video_usages/", UnknownRouteView.as_view(), @@ -32,4 +39,14 @@ UnknownRouteView.as_view(), name="course_video_usage_detail_unmatched", ), + path( + "courses//video_archives/", + UnknownRouteView.as_view(), + name="course_video_archive_list_unmatched", + ), + re_path( + r"^courses/(?P.+)/video_archives/(?P.*)/$", + UnknownRouteView.as_view(), + name="course_video_archive_detail_unmatched", + ), ] diff --git a/cms/djangoapps/contentstore/rest_api/v2/serializers/video_archives.py b/cms/djangoapps/contentstore/rest_api/v2/serializers/video_archives.py new file mode 100644 index 000000000000..2885d39b3c5a --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/serializers/video_archives.py @@ -0,0 +1,35 @@ +"""Serializers for zip archives of a course's videos.""" + +from rest_framework import serializers + + +class VideoArchiveFileSerializer(serializers.Serializer): # pylint: disable=abstract-method + """One video to put in the archive.""" + + # Values are used exactly as sent: the URL is matched character for + # character against the course's video URLs, and the name becomes the entry + # name in the archive. + url = serializers.CharField( + trim_whitespace=False, + help_text=( + "Address of one of the course's encoded video files, exactly as the course's video " + "listing reports it. Any other address is refused." + ), + ) + name = serializers.CharField( + trim_whitespace=False, + allow_blank=True, + help_text=( + "File name of the video inside the archive. When it does not already end with the " + "extension of the video's content type, that extension is appended." + ), + ) + + +class VideoArchiveRequestSerializer(serializers.Serializer): # pylint: disable=abstract-method + """The videos to bundle into one archive.""" + + files = VideoArchiveFileSerializer( + many=True, + help_text="The videos to include, in archive order. An empty list gives an empty archive.", + ) diff --git a/cms/djangoapps/contentstore/rest_api/v2/video_management_service.py b/cms/djangoapps/contentstore/rest_api/v2/video_management_service.py index 30c53dd10a53..32a49b65b3d0 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/video_management_service.py +++ b/cms/djangoapps/contentstore/rest_api/v2/video_management_service.py @@ -1,8 +1,8 @@ -"""Service layer for the video usage resources of a course.""" +"""Service layer for the video usage and video archive resources of a course.""" from rest_framework.exceptions import NotFound -from cms.djangoapps.contentstore.video_storage_handlers import get_video_usage_path +from cms.djangoapps.contentstore.video_storage_handlers import create_video_zip, get_video_usage_path from openedx.core.djangoapps.content.course_overviews.models import CourseOverview @@ -28,3 +28,14 @@ def get_course_video_usages(course_key, edx_video_id): """ ensure_course_exists(course_key) return get_video_usage_path(course_key, edx_video_id) + + +def create_course_video_archive(course_key, files): + """ + Return a streaming zip response holding the requested videos of the course. + + Every ``files[].url`` must be one of the course's own encoded-video URLs; + otherwise ``ValidationError`` is raised before anything is fetched. The + archive is assembled while it is sent and is not stored. + """ + return create_video_zip(str(course_key), files) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_archives.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_archives.py new file mode 100644 index 000000000000..13d25ed8df50 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_archives.py @@ -0,0 +1,1024 @@ +""" +Unit tests for the course video archives API. +""" +import io +import json +import zipfile +from datetime import datetime +from unittest.mock import MagicMock, patch +from urllib.parse import urlencode + +import ddt +import pytest +import pytz +import requests +from django.core.signals import got_request_exception +from django.urls import Resolver404, resolve, reverse +from edx_rest_framework_extensions.auth.jwt.tests.utils import generate_jwt +from edx_rest_framework_extensions.testing import assert_error_envelope +from edxval.api import create_profile, create_video, remove_video_for_course +from rest_framework import status +from rest_framework.test import APIClient + +from cms.djangoapps.contentstore.rest_api.v1.views.unknown_route import UnknownRouteView +from cms.djangoapps.contentstore.rest_api.v1.views.videos import VideoDownloadThrottle +from cms.djangoapps.contentstore.rest_api.v2.views.video_archives import CourseVideoArchiveView +from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from common.djangoapps.student.roles import ( + CourseInstructorRole, + CourseLimitedStaffRole, + CourseStaffRole, +) +from common.djangoapps.student.tests.factories import UserFactory +from openedx.core.djangoapps.oauth_dispatch.tests.factories import AccessTokenFactory, ApplicationFactory +from xmodule.modulestore.tests.factories import CourseFactory + +LIST_URL_NAME = "authoring_v2:course_video_archive_list" +LEGACY_URL_NAME = "cms.djangoapps.contentstore:v1:video_usage" + +MISSING_COURSE_KEY = "course-v1:Org+Missing+Run" +CCX_COURSE_KEY = "ccx-v1:edX+DemoX+Demo_Course+ccx@1" + +# Keys the course_key path converter accepts that carry only a content version, +# with no organization, course or run. +VERSION_ONLY_COURSE_KEYS = ( + "course-v1:version@" + "a" * 24, + "course-v1:branch@published-branch+version@" + "a" * 24, + "ccx-v1:version@" + "a" * 24 + "+ccx@1", + "library-v1:version@" + "a" * 24, +) + +FIRST_URL = "http://video-store.example.com/profile1/first.mp4" +SECOND_URL = "http://video-store.example.com/profile2/second.webm" +OTHER_COURSE_URL = "http://video-store.example.com/profile1/other-course.mp4" +SHARED_URL = "http://video-store.example.com/profile1/shared.mp4" +# An internal address an attacker might try to make the server fetch. +SSRF_URL = "http://169.254.169.254/latest/meta-data/" + +FETCH = "cms.djangoapps.contentstore.video_storage_handlers.requests.get" + +# The archive builder each address calls. +SERVICE_ARCHIVE_BUILDER = "cms.djangoapps.contentstore.rest_api.v2.video_management_service.create_video_zip" +LEGACY_ARCHIVE_BUILDER = "cms.djangoapps.contentstore.rest_api.v1.views.videos.create_video_zip" + +# How the error handling publishes a required member missing from a list entry. +MISSING_MEMBER_ERROR = "{{'{member}': [ErrorDetail(string='This field is required.', code='required')]}}" + +# The size of a zip archive with no entries: the end-of-central-directory record. +EMPTY_ARCHIVE_SIZE = 22 +# The signature opening each record of a zip archive's central directory, which +# follows the last entry. +CENTRAL_DIRECTORY_SIGNATURE = b"PK\x01\x02" + + +def upstream_response(payload=b"video-bytes", content_type="video/mp4"): + """A stand-in for a streamed video fetch answering with ``payload``.""" + response = MagicMock() + response.iter_content = lambda chunk_size=None: iter([payload]) + response.headers = {"Content-Type": content_type} + return response + + +def failing_upstream_response(): + """A stand-in for a video fetch that the video store answers with a server error.""" + response = upstream_response() + response.raise_for_status.side_effect = requests.HTTPError("500 Server Error") + return response + + +def read_into(received, stream): + """Append each chunk of ``stream`` to ``received`` as it arrives.""" + for chunk in stream: + received.append(chunk) + + +def archive_entries(response): + """Return {entry name: bytes} of the streamed zip ``response``.""" + with zipfile.ZipFile(io.BytesIO(b"".join(response.streaming_content))) as archive: + return {name: archive.read(name) for name in archive.namelist()} + + +class VideoArchiveTestBase(CourseTestCase): + """A course with two encoded videos, and a second course with one of its own.""" + + # Isolates the throttle counters between tests. + ENABLED_CACHES = ["default"] + + def setUp(self): + super().setUp() + create_profile("profile1") + create_profile("profile2") + self.create_course_video("first", self.course.id, [("profile1", FIRST_URL), ("profile2", SECOND_URL)]) + self.other_course = CourseFactory.create() + self.create_course_video("other", self.other_course.id, [("profile1", OTHER_COURSE_URL)]) + + def create_course_video(self, edx_video_id, course_key, encodings, also_in=()): + create_video({ + "edx_video_id": edx_video_id, + "client_video_id": f"{edx_video_id}.mp4", + "duration": 42.0, + "status": "file_complete", + "courses": [str(key) for key in (course_key, *also_in)], + "created": datetime.now(pytz.utc), + "encoded_videos": [ + {"profile": profile, "url": url, "file_size": 1600, "bitrate": 100} + for profile, url in encodings + ], + }) + + def url(self, course_key=None): + return reverse(LIST_URL_NAME, kwargs={"course_key": course_key or self.course.id}) + + def legacy_url(self, course_key=None): + # The legacy download route shares its name with the usage route and is + # the one reverse() picks when no video id is given. + return reverse(LEGACY_URL_NAME, kwargs={"course_id": course_key or self.course.id}) + + def client_for(self, user): + client = APIClient() + if user is not None: + client.force_authenticate(user=user) + return client + + def user_with_role(self, role_class, course_key=None): + user = UserFactory.create() + role_class(course_key or self.course.id).add_users(user) + return user + + def post(self, client, files, course_key=None, **extra): + return client.post(self.url(course_key), {"files": files}, format="json", **extra) + + def put_legacy(self, client, files, course_key=None, **extra): + return client.put(self.legacy_url(course_key), {"files": files}, format="json", **extra) + + +@patch(FETCH) +class VideoArchiveResponseTest(VideoArchiveTestBase): + """The archive holds the requested videos under the requested names.""" + + def test_the_archive_is_sent_as_a_zip_attachment(self, fetch): + fetch.return_value = upstream_response() + + response = self.post(self.client_for(self.user), [{"url": FIRST_URL, "name": "Lecture 1.mp4"}]) + + assert response.status_code == status.HTTP_200_OK + assert response["Content-Type"] == "application/zip" + assert response["Content-Disposition"] == f"attachment; filename={self.course.id}_videos.zip" + assert archive_entries(response) == {"Lecture 1.mp4": b"video-bytes"} + fetch.assert_called_once_with(FIRST_URL, stream=True, allow_redirects=True) + + def test_a_name_without_an_extension_gets_one_from_the_content_type(self, fetch): + fetch.return_value = upstream_response(content_type="video/mp4") + + response = self.post(self.client_for(self.user), [{"url": FIRST_URL, "name": "Lecture 1"}]) + + assert list(archive_entries(response)) == ["Lecture 1.mp4"] + + def test_an_unknown_content_type_leaves_the_name_alone(self, fetch): + fetch.return_value = upstream_response(content_type="application/x-not-a-real-type") + + response = self.post(self.client_for(self.user), [{"url": FIRST_URL, "name": "Lecture 1"}]) + + assert list(archive_entries(response)) == ["Lecture 1"] + + def test_several_videos_are_archived_in_request_order(self, fetch): + fetch.side_effect = [ + upstream_response(b"first-bytes", "video/mp4"), + upstream_response(b"second-bytes", "video/webm"), + ] + + response = self.post(self.client_for(self.user), [ + {"url": FIRST_URL, "name": "one.mp4"}, + {"url": SECOND_URL, "name": "two"}, + ]) + + entries = archive_entries(response) + assert list(entries) == ["one.mp4", "two.webm"] + assert entries == {"one.mp4": b"first-bytes", "two.webm": b"second-bytes"} + + def test_an_empty_request_gives_an_empty_archive(self, fetch): + response = self.post(self.client_for(self.user), []) + + body = b"".join(response.streaming_content) + assert response.status_code == status.HTTP_200_OK + assert len(body) == EMPTY_ARCHIVE_SIZE + assert zipfile.ZipFile(io.BytesIO(body)).namelist() == [] + fetch.assert_not_called() + + def test_the_name_is_used_exactly_as_sent(self, fetch): + fetch.return_value = upstream_response() + + response = self.post(self.client_for(self.user), [{"url": FIRST_URL, "name": " spaced.mp4 "}]) + + assert list(archive_entries(response)) == [" spaced.mp4 .mp4"] + + +@patch(FETCH) +class VideoArchiveAllowlistTest(VideoArchiveTestBase): + """Only the course's own video addresses are ever fetched.""" + + def assert_refused_without_fetching(self, response, fetch, address): + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"non_field_errors": [f"Invalid video download url: {address}"]} + fetch.assert_not_called() + + def test_an_address_outside_the_course_is_refused(self, fetch): + response = self.post(self.client_for(self.user), [{"url": SSRF_URL, "name": "x"}]) + + self.assert_refused_without_fetching(response, fetch, SSRF_URL) + + def test_a_video_of_another_course_is_refused(self, fetch): + response = self.post(self.client_for(self.user), [{"url": OTHER_COURSE_URL, "name": "x.mp4"}]) + + self.assert_refused_without_fetching(response, fetch, OTHER_COURSE_URL) + + def test_one_bad_address_refuses_the_whole_request(self, fetch): + response = self.post(self.client_for(self.user), [ + {"url": FIRST_URL, "name": "one.mp4"}, + {"url": SSRF_URL, "name": "x"}, + ]) + + self.assert_refused_without_fetching(response, fetch, SSRF_URL) + + def test_an_address_is_matched_exactly(self, fetch): + padded = f" {FIRST_URL}" + + response = self.post(self.client_for(self.user), [{"url": padded, "name": "x"}]) + + self.assert_refused_without_fetching(response, fetch, padded) + + def test_the_legacy_address_refuses_the_same_request(self, fetch): + response = self.put_legacy(self.client_for(self.user), [{"url": SSRF_URL, "name": "x"}]) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json() == {"developer_message": [f"Invalid video download url: {SSRF_URL}"]} + fetch.assert_not_called() + + def share_a_video_then_remove_it_from_this_course(self): + """Give both courses one video, then remove it from this course only.""" + self.create_course_video( + "shared", self.course.id, [("profile1", SHARED_URL)], also_in=[self.other_course.id], + ) + remove_video_for_course(str(self.course.id), "shared") + + def test_a_video_removed_from_the_course_is_refused(self, fetch): + self.share_a_video_then_remove_it_from_this_course() + + response = self.post(self.client_for(self.user), [{"url": SHARED_URL, "name": "x.mp4"}]) + + self.assert_refused_without_fetching(response, fetch, SHARED_URL) + + def test_the_legacy_address_refuses_a_video_removed_from_the_course(self, fetch): + self.share_a_video_then_remove_it_from_this_course() + + response = self.put_legacy(self.client_for(self.user), [{"url": SHARED_URL, "name": "x.mp4"}]) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json() == {"developer_message": [f"Invalid video download url: {SHARED_URL}"]} + fetch.assert_not_called() + + def test_a_video_removed_from_one_course_is_still_archived_by_a_course_that_keeps_it(self, fetch): + self.share_a_video_then_remove_it_from_this_course() + fetch.return_value = upstream_response() + + response = self.post( + self.client_for(self.user), [{"url": SHARED_URL, "name": "x.mp4"}], course_key=self.other_course.id, + ) + + assert archive_entries(response) == {"x.mp4": b"video-bytes"} + fetch.assert_called_once_with(SHARED_URL, stream=True, allow_redirects=True) + + +@ddt.ddt +@patch(FETCH) +class VideoArchiveRequestValidationTest(VideoArchiveTestBase): + """ + A malformed body is refused with a 400 before anything is fetched. + + The error handling publishes the error of a list entry as the Python text of + that entry's errors, with "{}" for each valid entry, rather than mapping the + entry's path to its messages; and the detail of every validation error is + the Python text of all the errors. The exact text is pinned, so a change in + that formatting shows up here. + """ + + def assert_invalid(self, response, fetch, field="files"): + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert list(envelope["errors"]) == [field] + fetch.assert_not_called() + return envelope + + def test_a_missing_list_is_refused(self, fetch): + response = self.client_for(self.user).post(self.url(), {}, format="json") + + envelope = self.assert_invalid(response, fetch) + assert envelope["errors"]["files"] == ["This field is required."] + + def test_the_detail_of_a_validation_error_is_the_text_of_all_the_errors(self, fetch): + response = self.client_for(self.user).post(self.url(), {}, format="json") + + envelope = self.assert_invalid(response, fetch) + assert envelope["detail"] == "{'files': [ErrorDetail(string='This field is required.', code='required')]}" + + def test_a_list_of_the_wrong_type_is_refused(self, fetch): + response = self.client_for(self.user).post(self.url(), {"files": "x"}, format="json") + + envelope = self.assert_invalid(response, fetch) + assert envelope["errors"]["files"] == [ + "{'non_field_errors': [ErrorDetail(string='Expected a list of items but got type \"str\".', " + "code='not_a_list')]}" + ] + + @ddt.data("url", "name") + def test_an_entry_missing_a_member_is_refused(self, member, fetch): + entry = {"url": FIRST_URL, "name": "one.mp4"} + del entry[member] + + response = self.post(self.client_for(self.user), [entry]) + + envelope = self.assert_invalid(response, fetch) + assert envelope["errors"]["files"] == [MISSING_MEMBER_ERROR.format(member=member)] + + def test_a_valid_entry_beside_an_invalid_one_is_published_as_an_empty_map(self, fetch): + response = self.post(self.client_for(self.user), [{"url": FIRST_URL, "name": "one.mp4"}, {"name": "two.mp4"}]) + + envelope = self.assert_invalid(response, fetch) + assert envelope["errors"]["files"] == ["{}", MISSING_MEMBER_ERROR.format(member="url")] + + def test_a_null_entry_is_refused(self, fetch): + response = self.post(self.client_for(self.user), [None]) + + envelope = self.assert_invalid(response, fetch) + assert envelope["errors"]["files"] == ["[ErrorDetail(string='This field may not be null.', code='null')]"] + + def test_a_null_name_is_refused(self, fetch): + response = self.post(self.client_for(self.user), [{"url": FIRST_URL, "name": None}]) + + envelope = self.assert_invalid(response, fetch) + assert envelope["errors"]["files"] == [ + "{'name': [ErrorDetail(string='This field may not be null.', code='null')]}" + ] + + def test_malformed_json_is_refused(self, fetch): + response = self.client_for(self.user).post(self.url(), "{not json", content_type="application/json") + + assert_error_envelope(response, expected_status=400) + fetch.assert_not_called() + + +@patch(FETCH) +class VideoArchiveMediaTypeTest(VideoArchiveTestBase): + """A body that is not JSON is refused before anything is fetched.""" + + # The indexed syntax the default form parsers would turn back into the list. + FORM_FIELDS = {"files[0]url": FIRST_URL, "files[0]name": "one.mp4"} + + def assert_unsupported(self, response, fetch): + # The status and the envelope are pinned, not the error-type slug, which + # the shared error catalog decides for every API. + assert_error_envelope(response, expected_status=status.HTTP_415_UNSUPPORTED_MEDIA_TYPE) + fetch.assert_not_called() + + def test_a_form_encoded_body_is_refused(self, fetch): + fetch.return_value = upstream_response() + + response = self.client_for(self.user).post( + self.url(), urlencode(self.FORM_FIELDS), content_type="application/x-www-form-urlencoded" + ) + + self.assert_unsupported(response, fetch) + + def test_a_plain_text_body_is_refused(self, fetch): + fetch.return_value = upstream_response() + + response = self.client_for(self.user).post( + self.url(), json.dumps({"files": [{"url": FIRST_URL, "name": "one.mp4"}]}), content_type="text/plain" + ) + + self.assert_unsupported(response, fetch) + + def test_a_multipart_body_is_refused(self, fetch): + fetch.return_value = upstream_response() + + response = self.client_for(self.user).post(self.url(), self.FORM_FIELDS, format="multipart") + + # Studio's request-tracking middleware reads the form fields of a + # multipart request before the view runs, which leaves the JSON parser + # an empty body: the request is refused as one without files. + envelope = assert_error_envelope(response, expected_status=400, expected_type_slug="validation") + assert envelope["errors"] == {"files": ["This field is required."]} + fetch.assert_not_called() + + def test_the_view_parses_json_only(self, fetch): + assert [parser.media_type for parser in CourseVideoArchiveView().get_parsers()] == ["application/json"] + + def test_the_legacy_address_fails_on_a_form_encoded_body(self, fetch): + client = self.client_for(self.user) + client.raise_request_exception = False + + response = client.put( + self.legacy_url(), urlencode(self.FORM_FIELDS), content_type="application/x-www-form-urlencoded" + ) + + assert response.status_code == status.HTTP_500_INTERNAL_SERVER_ERROR + fetch.assert_not_called() + + +@patch(FETCH) +class VideoArchiveAcceptHeaderTest(VideoArchiveTestBase): + """ + The archive is sent to a request that also accepts JSON, in which errors are + sent; a request that accepts the archive's type alone is refused. + """ + + def test_a_request_accepting_the_archive_and_json_gets_the_archive(self, fetch): + fetch.return_value = upstream_response() + + response = self.post( + self.client_for(self.user), + [{"url": FIRST_URL, "name": "one.mp4"}], + HTTP_ACCEPT="application/zip, application/json", + ) + + assert response.status_code == status.HTTP_200_OK + assert response["Content-Type"] == "application/zip" + assert archive_entries(response) == {"one.mp4": b"video-bytes"} + + def test_a_request_accepting_only_the_archive_is_refused_before_anything_is_fetched(self, fetch): + fetch.return_value = upstream_response() + + response = self.post( + self.client_for(self.user), [{"url": FIRST_URL, "name": "one.mp4"}], HTTP_ACCEPT="application/zip" + ) + + # The status and the envelope are pinned, not the error-type slug, which + # the shared error catalog decides for every API. + assert_error_envelope(response, expected_status=status.HTTP_406_NOT_ACCEPTABLE) + fetch.assert_not_called() + + +@ddt.ddt +@patch(FETCH) +class VideoArchiveAuthorizationTest(VideoArchiveTestBase): + """Studio read access to the course is required; everyone else is refused before any fetch.""" + + def assert_allowed(self, user, fetch): + """Assert ``user`` receives the archive of one requested video.""" + fetch.return_value = upstream_response() + response = self.post(self.client_for(user), [{"url": FIRST_URL, "name": "one.mp4"}]) + assert response.status_code == status.HTTP_200_OK + assert response["Content-Type"] == "application/zip" + assert archive_entries(response) == {"one.mp4": b"video-bytes"} + + def test_global_staff_is_allowed(self, fetch): + self.assert_allowed(self.user, fetch) + + def test_course_staff_is_allowed(self, fetch): + self.assert_allowed(self.user_with_role(CourseStaffRole), fetch) + + def test_course_instructor_is_allowed(self, fetch): + self.assert_allowed(self.user_with_role(CourseInstructorRole), fetch) + + def assert_refused(self, response, fetch, expected_status, slug): + assert_error_envelope(response, expected_status=expected_status, expected_type_slug=slug) + fetch.assert_not_called() + + def test_anonymous_is_refused(self, fetch): + response = self.post(self.client_for(None), [{"url": FIRST_URL, "name": "one.mp4"}]) + + self.assert_refused(response, fetch, 401, "authn") + assert response["WWW-Authenticate"] == 'JWT realm="api"' + + def test_user_without_a_role_is_refused(self, fetch): + response = self.post(self.client_for(UserFactory.create()), [{"url": FIRST_URL, "name": "one.mp4"}]) + + self.assert_refused(response, fetch, 403, "authz") + + def test_limited_staff_is_refused(self, fetch): + user = self.user_with_role(CourseLimitedStaffRole) + + response = self.post(self.client_for(user), [{"url": FIRST_URL, "name": "one.mp4"}]) + + self.assert_refused(response, fetch, 403, "authz") + + def test_staff_of_another_course_is_refused(self, fetch): + user = self.user_with_role(CourseStaffRole, course_key=self.other_course.id) + + response = self.post(self.client_for(user), [{"url": FIRST_URL, "name": "one.mp4"}]) + + self.assert_refused(response, fetch, 403, "authz") + + def test_ccx_course_is_refused_even_to_global_staff(self, fetch): + response = self.post(self.client_for(self.user), [], course_key=CCX_COURSE_KEY) + + self.assert_refused(response, fetch, 403, "authz") + + def test_missing_course_is_refused_to_a_caller_without_access(self, fetch): + response = self.post(self.client_for(UserFactory.create()), [], course_key=MISSING_COURSE_KEY) + + self.assert_refused(response, fetch, 403, "authz") + + def test_missing_course_is_reported_to_global_staff(self, fetch): + response = self.post(self.client_for(self.user), [], course_key=MISSING_COURSE_KEY) + + self.assert_refused(response, fetch, 404, "not-found") + + def test_a_missing_course_is_reported_before_the_body_is_read(self, fetch): + response = self.client_for(self.user).post(self.url(MISSING_COURSE_KEY), {}, format="json") + + self.assert_refused(response, fetch, 404, "not-found") + + @ddt.data(*VERSION_ONLY_COURSE_KEYS) + def test_a_key_carrying_only_a_version_is_not_found_by_global_staff(self, key, fetch): + response = self.post(self.client_for(self.user), [{"url": FIRST_URL, "name": "one.mp4"}], course_key=key) + + self.assert_refused(response, fetch, 404, "not-found") + assert response.json()["detail"] == "Not found." + + @ddt.data(*VERSION_ONLY_COURSE_KEYS) + def test_a_key_carrying_only_a_version_is_not_found_by_a_caller_without_a_role(self, key, fetch): + response = self.post( + self.client_for(UserFactory.create()), [{"url": FIRST_URL, "name": "one.mp4"}], course_key=key, + ) + + self.assert_refused(response, fetch, 404, "not-found") + assert response.json()["detail"] == "Not found." + + @ddt.data(*VERSION_ONLY_COURSE_KEYS) + def test_a_key_carrying_only_a_version_still_needs_authentication(self, key, fetch): + response = self.post(self.client_for(None), [{"url": FIRST_URL, "name": "one.mp4"}], course_key=key) + + self.assert_refused(response, fetch, 401, "authn") + + def test_the_legacy_address_has_no_route_for_a_version_only_key(self, fetch): + # The legacy pattern needs a separator inside the key. + with pytest.raises(Resolver404): + resolve(f"/api/contentstore/v1/videos/{VERSION_ONLY_COURSE_KEYS[0]}/download") + + def test_the_view_declares_its_checks_in_order(self, fetch): + # The JWT middleware appends its own restricted-application check to + # views that accept JWTs, so only the declared head of the list is fixed. + assert [klass.__name__ for klass in CourseVideoArchiveView.permission_classes[:3]] == [ + "IsAuthenticated", "HasFullCourseKey", "HasStudioReadAccess", + ] + + +class VideoArchiveUnexpectedFailureTest(VideoArchiveTestBase): + """ + An unexpected failure while the archive is set up, at both addresses. + + The new address answers with the generic 500 body from inside the view, so + the exception never reaches Django: ``got_request_exception`` is not sent, + and the only log record is Django's one-line 500 notice, without the + traceback. The legacy address lets the exception reach Django, which sends + the signal and logs the traceback. Both sides are pinned, so a change in how + the error handling reports failures shows up here. + """ + + BODY = {"files": [{"url": FIRST_URL, "name": "one.mp4"}]} + + def request_while_failing(self, method, address, target): + """Send the body as global staff while ``target`` raises; return the response, signals and log records.""" + signals = [] + + def receiver(sender, **kwargs): + signals.append(sender) + + got_request_exception.connect(receiver) + self.addCleanup(got_request_exception.disconnect, receiver) + client = APIClient(raise_request_exception=False) + client.force_authenticate(user=self.user) + with patch(target, side_effect=RuntimeError("video store unreachable at edxval-3")): + with self.assertLogs(level="ERROR") as logs: + response = getattr(client, method)(address, self.BODY, format="json") + return response, signals, logs.records + + def test_the_failure_is_answered_with_the_generic_body(self): + response, __, __ = self.request_while_failing("post", self.url(), SERVICE_ARCHIVE_BUILDER) + + envelope = assert_error_envelope(response, expected_status=500, expected_type_slug="internal") + assert envelope["detail"] == "An unexpected error occurred. Please try again later." + assert "edxval-3" not in response.content.decode() + + def test_the_failure_does_not_reach_django(self): + __, signals, records = self.request_while_failing("post", self.url(), SERVICE_ARCHIVE_BUILDER) + + assert not signals + assert [(record.name, record.getMessage(), record.exc_info) for record in records] == [ + ("django.request", f"Internal Server Error: {self.url()}", None), + ] + + def test_the_legacy_address_hands_the_failure_to_django(self): + __, signals, records = self.request_while_failing("put", self.legacy_url(), LEGACY_ARCHIVE_BUILDER) + + assert len(signals) == 1 + assert "django.request" in { + record.name for record in records if record.exc_info and record.exc_info[0] is RuntimeError + } + + +@patch(FETCH) +class VideoArchiveAuthenticationTest(VideoArchiveTestBase): + """The endpoint uses the platform's default authentication.""" + + def test_session_caller_succeeds(self, fetch): + fetch.return_value = upstream_response() + client = APIClient() + assert client.login(username=self.user.username, password=self.user_password) + + response = self.post(client, [{"url": FIRST_URL, "name": "one.mp4"}]) + + assert archive_entries(response) == {"one.mp4": b"video-bytes"} + + def test_session_caller_needs_a_csrf_token(self, fetch): + client = APIClient(enforce_csrf_checks=True) + assert client.login(username=self.user.username, password=self.user_password) + + response = self.post(client, [{"url": FIRST_URL, "name": "one.mp4"}]) + legacy = self.put_legacy(client, [{"url": FIRST_URL, "name": "one.mp4"}]) + + envelope = assert_error_envelope(response, expected_status=403, expected_type_slug="authz") + assert envelope["detail"].startswith("CSRF Failed") + assert legacy.status_code == status.HTTP_403_FORBIDDEN + fetch.assert_not_called() + + def test_jwt_caller_succeeds(self, fetch): + fetch.return_value = upstream_response() + + response = self.post( + APIClient(), [{"url": FIRST_URL, "name": "one.mp4"}], + HTTP_AUTHORIZATION=f"JWT {generate_jwt(self.user)}", + ) + + assert archive_entries(response) == {"one.mp4": b"video-bytes"} + + def bearer_header(self, user): + token = AccessTokenFactory(user=user, application=ApplicationFactory()).token + return f"Bearer {token}" + + def test_bearer_token_is_refused(self, fetch): + response = self.post( + APIClient(), [{"url": FIRST_URL, "name": "one.mp4"}], HTTP_AUTHORIZATION=self.bearer_header(self.user), + ) + + assert_error_envelope(response, expected_status=401, expected_type_slug="authn") + fetch.assert_not_called() + + def test_the_legacy_address_still_accepts_a_bearer_token(self, fetch): + response = self.put_legacy(APIClient(), [], HTTP_AUTHORIZATION=self.bearer_header(self.user)) + + assert response.status_code == status.HTTP_200_OK + + def deactivated_global_staff(self): + """Log a global staff member in, then deactivate the account behind the live session.""" + password = "password-12345" + staff = UserFactory.create(is_staff=True, password=password) + session_client = APIClient() + assert session_client.login(username=staff.username, password=password) + staff.is_active = False + staff.save() + return staff, session_client + + def test_inactive_global_staff_is_refused_over_a_session(self, fetch): + __, session_client = self.deactivated_global_staff() + + response = self.post(session_client, [{"url": FIRST_URL, "name": "one.mp4"}]) + + assert_error_envelope(response, expected_status=401, expected_type_slug="authn") + fetch.assert_not_called() + + def test_inactive_global_staff_still_reaches_the_legacy_address_over_a_session(self, fetch): + __, session_client = self.deactivated_global_staff() + + response = self.put_legacy(session_client, []) + + assert response.status_code == status.HTTP_200_OK + + def test_inactive_global_staff_is_admitted_over_jwt(self, fetch): + staff, __ = self.deactivated_global_staff() + + response = self.post(APIClient(), [], HTTP_AUTHORIZATION=f"JWT {generate_jwt(staff)}") + + assert response.status_code == status.HTTP_200_OK + assert len(b"".join(response.streaming_content)) == EMPTY_ARCHIVE_SIZE + + def test_the_view_does_not_declare_authentication_classes(self, fetch): + assert "authentication_classes" not in CourseVideoArchiveView.__dict__ + assert [klass.__name__ for klass in CourseVideoArchiveView.authentication_classes] == [ + "DefaultJwtAuthentication", "DefaultSessionAuthentication", + ] + + +@patch(FETCH) +class VideoArchiveThrottleTest(VideoArchiveTestBase): + """Archive requests are rate-limited per user, in one budget with the legacy download address.""" + + def budget(self): + return VideoDownloadThrottle().num_requests + + def test_the_view_uses_the_download_throttle(self, fetch): + assert CourseVideoArchiveView.throttle_classes == (VideoDownloadThrottle,) + assert self.budget() == 12 + + def test_requests_past_the_budget_are_throttled(self, fetch): + client = self.client_for(self.user) + for __ in range(self.budget()): + assert self.post(client, []).status_code == status.HTTP_200_OK + + response = self.post(client, []) + + assert_error_envelope(response, expected_status=429, expected_type_slug="rate-limited") + assert response["Retry-After"] == "3600" + + def test_the_budget_is_shared_with_the_legacy_address(self, fetch): + client = self.client_for(self.user) + for __ in range(self.budget()): + assert self.put_legacy(client, []).status_code == status.HTTP_200_OK + + response = self.post(client, []) + + assert_error_envelope(response, expected_status=429, expected_type_slug="rate-limited") + + def test_the_budget_is_per_user(self, fetch): + client = self.client_for(self.user) + for __ in range(self.budget()): + self.post(client, []) + + response = self.post(self.client_for(self.user_with_role(CourseStaffRole)), []) + + assert response.status_code == status.HTTP_200_OK + + def test_a_refused_caller_spends_no_budget(self, fetch): + client = self.client_for(UserFactory.create()) + responses = [self.post(client, []) for __ in range(self.budget() + 2)] + + assert {response.status_code for response in responses} == {status.HTTP_403_FORBIDDEN} + + def test_the_legacy_address_spends_budget_on_a_refused_caller(self, fetch): + client = self.client_for(UserFactory.create()) + responses = [self.put_legacy(client, []) for __ in range(self.budget() + 1)] + + assert responses[-2].status_code == status.HTTP_403_FORBIDDEN + assert responses[-1].status_code == status.HTTP_429_TOO_MANY_REQUESTS + + +@ddt.ddt +@patch(FETCH) +class VideoArchiveParityTest(VideoArchiveTestBase): + """For the same request and caller, both addresses send the same archive.""" + + def pair(self, files, responses_for_each, course_key=None, user=None): + """POST to the new address and PUT to the legacy one, each fetch answered alike.""" + client = self.client_for(user or self.user) + with patch(FETCH, side_effect=list(responses_for_each())): + new = self.post(client, files, course_key) + new_body = b"".join(new.streaming_content) if new.status_code == 200 else None + with patch(FETCH, side_effect=list(responses_for_each())): + legacy = self.put_legacy(client, files, course_key) + legacy_body = b"".join(legacy.streaming_content) if legacy.status_code == 200 else None + return legacy, legacy_body, new, new_body + + def assert_same_archive(self, files, responses_for_each, expected_entries): + """Assert both addresses send byte-identical archives holding ``expected_entries``.""" + legacy, legacy_body, new, new_body = self.pair(files, responses_for_each) + assert (legacy.status_code, new.status_code) == (status.HTTP_200_OK, status.HTTP_200_OK) + for header in ("Content-Type", "Content-Disposition"): + assert new[header] == legacy[header] + assert new_body == legacy_body + with zipfile.ZipFile(io.BytesIO(new_body)) as archive: + assert {name: archive.read(name) for name in archive.namelist()} == expected_entries + + def test_a_name_with_an_extension(self, fetch): + self.assert_same_archive( + [{"url": FIRST_URL, "name": "one.mp4"}], + lambda: [upstream_response(b"a", "video/mp4")], + {"one.mp4": b"a"}, + ) + + def test_an_extension_from_the_content_type(self, fetch): + self.assert_same_archive( + [{"url": FIRST_URL, "name": "one"}], + lambda: [upstream_response(b"a", "video/mp4")], + {"one.mp4": b"a"}, + ) + + def test_an_unknown_content_type(self, fetch): + self.assert_same_archive( + [{"url": FIRST_URL, "name": "one"}], + lambda: [upstream_response(b"a", "application/x-not-a-real-type")], + {"one": b"a"}, + ) + + def test_several_files(self, fetch): + self.assert_same_archive( + [{"url": FIRST_URL, "name": "one.mp4"}, {"url": SECOND_URL, "name": "two"}], + lambda: [upstream_response(b"a", "video/mp4"), upstream_response(b"b", "video/webm")], + {"one.mp4": b"a", "two.webm": b"b"}, + ) + + def test_an_empty_list(self, fetch): + legacy, legacy_body, new, new_body = self.pair([], lambda: []) + + assert new_body == legacy_body + assert len(new_body) == EMPTY_ARCHIVE_SIZE + assert new["Content-Disposition"] == legacy["Content-Disposition"] == ( + f"attachment; filename={self.course.id}_videos.zip" + ) + + def received_until_the_failure(self, send, second_fetch, failure): + """ + Request two videos whose second fetch fails; return the bytes streamed before the failure. + + The status is sent before the archive, so the response is a 200 and the + failure surfaces while the body is being read. + """ + with patch(FETCH, side_effect=[upstream_response(b"first-bytes", "video/mp4"), second_fetch]) as fetch: + response = send( + self.client_for(self.user), + [{"url": FIRST_URL, "name": "one.mp4"}, {"url": SECOND_URL, "name": "two.webm"}], + ) + assert response.status_code == status.HTTP_200_OK + received = [] + with pytest.raises(failure): + read_into(received, response.streaming_content) + assert [call.args[0] for call in fetch.call_args_list] == [FIRST_URL, SECOND_URL] + return b"".join(received) + + @ddt.data( + # The video store answers the second fetch with a server error. + (failing_upstream_response, requests.HTTPError), + # The connection to the video store fails on the second fetch. + (lambda: requests.ConnectionError("Connection reset"), requests.ConnectionError), + ) + @ddt.unpack + def test_a_fetch_failing_part_way_stops_the_archive(self, second_fetch, failure, fetch): + new = self.received_until_the_failure(self.post, second_fetch(), failure) + legacy = self.received_until_the_failure(self.put_legacy, second_fetch(), failure) + with patch(FETCH, return_value=upstream_response(b"first-bytes", "video/mp4")): + response = self.post(self.client_for(self.user), [{"url": FIRST_URL, "name": "one.mp4"}]) + first_video_only = b"".join(response.streaming_content) + + # Both addresses stop after the first video's entry, before the + # archive's directory: what arrives is the archive of the first video + # alone, cut short, and no zip reader can open it. + assert new == legacy + assert first_video_only[:len(new)] == new + assert first_video_only[len(new):].startswith(CENTRAL_DIRECTORY_SIGNATURE) + with pytest.raises(zipfile.BadZipFile): + zipfile.ZipFile(io.BytesIO(new)) + + @ddt.data( + (None, status.HTTP_401_UNAUTHORIZED, {"developer_message"}), + ("no-role", status.HTTP_403_FORBIDDEN, {"developer_message"}), + ("missing-course", status.HTTP_404_NOT_FOUND, {"developer_message", "error_code"}), + ("bad-url", status.HTTP_400_BAD_REQUEST, {"developer_message"}), + ) + @ddt.unpack + def test_error_bodies(self, case, expected_status, legacy_keys, fetch): + client = { + None: self.client_for(None), + "no-role": self.client_for(UserFactory.create()), + }.get(case, self.client_for(self.user)) + course_key = MISSING_COURSE_KEY if case == "missing-course" else None + files = [{"url": SSRF_URL if case == "bad-url" else FIRST_URL, "name": "x"}] + + legacy = self.put_legacy(client, files, course_key) + new = self.post(client, files, course_key) + + assert legacy.status_code == new.status_code == expected_status + assert set(legacy.json()) == legacy_keys + expected_new_keys = {"type", "title", "status", "detail", "instance"} + if case == "bad-url": + expected_new_keys.add("errors") + assert set(new.json()) == expected_new_keys + fetch.assert_not_called() + + +class VideoArchiveQueryCountTest(VideoArchiveTestBase): + """ + The new address costs no more queries than the legacy one for the same caller. + + Each error answered by the new address ends with one extra statement, a + rollback of the request's savepoint, issued by the standard error handling. + """ + + #: Set from the pytest fixture below, which class-based tests cannot request directly. + django_assert_num_queries = None + + @pytest.fixture(autouse=True) + def _assert_num_queries(self, django_assert_num_queries): + self.django_assert_num_queries = django_assert_num_queries + + def measure(self, client, files, legacy_count, new_count, course_key=None): + """Warm the per-process caches, then count the queries of each address.""" + with patch(FETCH, return_value=upstream_response()): + self.put_legacy(client, files, course_key) + self.post(client, files, course_key) + with self.django_assert_num_queries(legacy_count): + self.put_legacy(client, files, course_key) + with self.django_assert_num_queries(new_count): + self.post(client, files, course_key) + + def test_global_staff(self): + self.measure(self.client_for(self.user), [{"url": FIRST_URL, "name": "a"}], 11, 11) + + def test_refused_address(self): + self.measure(self.client_for(self.user), [{"url": SSRF_URL, "name": "a"}], 11, 12) + + def test_refused_caller(self): + self.measure(self.client_for(UserFactory.create()), [], 8, 7) + + def test_missing_course(self): + self.measure(self.client_for(self.user), [], 8, 9, course_key=MISSING_COURSE_KEY) + + def test_anonymous(self): + self.measure(self.client_for(None), [], 5, 6) + + +@ddt.ddt +class VideoArchiveUrlContractTest(VideoArchiveTestBase): + """The archive address, and how unusable addresses and requests are answered.""" + + def test_conforming_address(self): + assert self.url() == f"/api/authoring/v2/courses/{self.course.id}/video_archives/" + + def test_the_address_resolves_to_the_view(self): + match = resolve(self.url()) + + assert match.func.cls is CourseVideoArchiveView + assert match.kwargs == {"course_key": self.course.id} + + def test_the_legacy_address_is_unchanged(self): + assert self.legacy_url() == f"/api/contentstore/v1/videos/{self.course.id}/download" + assert resolve(self.legacy_url()).func.view_class.__name__ == "VideoDownloadView" + + def assert_json_not_found(self, address): + """POST to ``address`` as a JSON client; assert the 404 error envelope and return it.""" + response = self.client_for(self.user).post( + address, {"files": []}, format="json", HTTP_ACCEPT="application/json", + ) + assert response["Content-Type"] == "application/json" + return assert_error_envelope(response, expected_status=404, expected_type_slug="not-found") + + def test_malformed_course_key_is_answered_with_the_error_envelope(self): + self.assert_json_not_found("/api/authoring/v2/courses/not-a-key/video_archives/") + + def test_deprecated_course_key_is_answered_with_the_error_envelope(self): + self.assert_json_not_found("/api/authoring/v2/courses/Org/Course/Run/video_archives/") + + def test_a_block_key_in_place_of_a_course_key_is_answered_with_the_error_envelope(self): + self.assert_json_not_found("/api/authoring/v2/courses/i4x://Org/Course/video/abc/video_archives/") + + def test_an_unusable_course_key_is_claimed_by_the_fallback_route(self): + address = "/api/authoring/v2/courses/Org/Course/Run/video_archives/" + + assert resolve(address).func.cls is UnknownRouteView + assert reverse( + "authoring_v2:course_video_archive_list_unmatched", kwargs={"course_key": "Org/Course/Run"}, + ) == address + assert reverse( + "authoring_v2:course_video_archive_detail_unmatched", + kwargs={"course_key": "Org/Course/Run", "subpath": "abc"}, + ) == f"{address}abc/" + + def test_the_legacy_address_still_accepts_a_deprecated_course_key(self): + match = resolve("/api/contentstore/v1/videos/Org/Course/Run/download") + + assert match.func.view_class.__name__ == "VideoDownloadView" + assert match.kwargs == {"course_id": "Org/Course/Run"} + + @ddt.data("video_archives/abc/", "video_archives//", "video_archives/abc/def/") + def test_an_address_below_the_archives_is_answered_with_the_error_envelope(self, rest): + address = f"/api/authoring/v2/courses/{self.course.id}/{rest}" + + assert resolve(address).func.cls is UnknownRouteView + self.assert_json_not_found(address) + + def test_an_address_below_the_archives_without_the_trailing_slash_is_redirected(self): + address = f"/api/authoring/v2/courses/{self.course.id}/video_archives/abc" + + response = self.client_for(self.user).post(address, {"files": []}, format="json") + + assert response.status_code == status.HTTP_301_MOVED_PERMANENTLY + assert response["Location"] == f"{address}/" + + def test_the_address_requires_the_trailing_slash(self): + with pytest.raises(Resolver404): + resolve(self.url().rstrip("/")) + + def test_a_post_without_the_trailing_slash_is_redirected(self): + response = self.client_for(self.user).post(self.url().rstrip("/"), {"files": []}, format="json") + + assert response.status_code == status.HTTP_301_MOVED_PERMANENTLY + assert response["Location"] == self.url() + + def test_other_methods_are_refused(self): + client = self.client_for(self.user) + for response in (client.get(self.url()), client.put(self.url(), {"files": []}, format="json")): + assert_error_envelope(response, expected_status=405) + assert response["Allow"] == "POST, OPTIONS" + + def test_an_accept_header_admitting_only_zip_is_refused_before_authentication(self): + response = self.post(self.client_for(None), [], HTTP_ACCEPT="application/zip") + + assert_error_envelope(response, expected_status=406) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py index 803e0fed0617..baf0366c3354 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py @@ -1,5 +1,5 @@ """ -Contract tests shared by the course video APIs. +Contract tests shared by the course video usage and video archive APIs. """ import functools import json @@ -23,6 +23,7 @@ import cms.envs from cms.djangoapps.contentstore.rest_api.v2.serializers.video_usages import VideoUsageLocationSerializer +from cms.djangoapps.contentstore.rest_api.v2.views.video_archives import CourseVideoArchiveView from cms.djangoapps.contentstore.rest_api.v2.views.video_usages import CourseVideoUsageViewSet from cms.djangoapps.contentstore.tests.utils import CourseTestCase from cms.djangoapps.contentstore.views.permissions import HasFullCourseKey, HasStudioReadAccess @@ -36,6 +37,7 @@ from xmodule.modulestore.tests.factories import CourseFactory USAGE_PATH = "/api/authoring/v2/courses/{course_key}/video_usages/{edx_video_id}/" +ARCHIVE_PATH = "/api/authoring/v2/courses/{course_key}/video_archives/" LEGACY_USAGE_PATH = "/api/contentstore/v1/videos/{course_id}/{edx_video_id}/usage" LEGACY_DOWNLOAD_PATH = "/api/contentstore/v1/videos/{course_id}/download" @@ -133,22 +135,26 @@ class VideoRoutesTest(TestCase): COURSE_KEY = "course-v1:edX+DemoX+Demo_Course" - # The routes of this resource, in order. Other resources share the + # The routes of these two resources, in order. Other resources share the # namespace, so only these are pinned. ROUTE_NAMES = ( "course_video_usage_detail", + "course_video_archive_list", "course_video_usage_list_unmatched", "course_video_usage_detail_unmatched", + "course_video_archive_list_unmatched", + "course_video_archive_detail_unmatched", ) - # Addresses outside the collection, some named like it, that another + # Addresses outside the two collections, some named like them, that another # resource may serve. Neither fallback may claim them. - ADDRESSES_OUTSIDE_THE_COLLECTION = ( + ADDRESSES_OUTSIDE_THE_COLLECTIONS = ( f"/api/authoring/v2/courses/{COURSE_KEY}/", f"/api/authoring/v2/courses/{COURSE_KEY}/textbooks/", "/api/authoring/v2/courses/Org/Course/Run/textbooks/", f"/api/authoring/v2/courses/{COURSE_KEY}/textbooks/video_usages_report/", f"/api/authoring/v2/courses/{COURSE_KEY}/video_usage/abc-123/", + f"/api/authoring/v2/courses/{COURSE_KEY}/all_video_archives/", "/api/authoring/v2/video_usages/", ) @@ -178,10 +184,10 @@ def test_the_new_route_names_are_unique_and_carry_no_version(self): assert resolver.app_name == "authoring_v2" assert not [name for name in names if "v1" in name or "v2" in name] - def test_the_fallbacks_claim_nothing_outside_the_collection(self): + def test_the_fallbacks_claim_nothing_outside_the_two_collections(self): fallbacks = {f"authoring_v2:{name}" for name in self.ROUTE_NAMES if name.endswith("_unmatched")} claimed = [] - for address in self.ADDRESSES_OUTSIDE_THE_COLLECTION: + for address in self.ADDRESSES_OUTSIDE_THE_COLLECTIONS: try: match = resolve(address) except Resolver404: @@ -270,7 +276,7 @@ def test_a_view_without_a_course_key_passes(self): class CmsSchemaHookTest(TestCase): - """The schema hooks admit the new addresses and deprecate exactly the superseded one.""" + """The schema hooks admit the new addresses and deprecate exactly the two superseded ones.""" def test_filter_admits_the_authoring_prefix(self): admitted = cms_api_filter([ @@ -308,39 +314,41 @@ def test_filter_admits_the_course_discussions_switch_and_nothing_else_under_cour "/api/courses/{course_key_string}/bulk_enable_disable_discussions", ] - def test_the_superseded_paths_include_the_usage_operation(self): + def test_the_superseded_paths_include_the_two_video_operations(self): assert [ prefix for prefix in SUPERSEDED_PATH_PREFIXES if prefix in (LEGACY_USAGE_PATH, LEGACY_DOWNLOAD_PATH) - ] == [LEGACY_USAGE_PATH] + ] == [LEGACY_USAGE_PATH, LEGACY_DOWNLOAD_PATH] - # Operations of other resources that nothing supersedes, next to the - # superseded one or elsewhere in Studio, and the new address. Marking any + # Operations of other resources that nothing supersedes, next to the two + # superseded ones or elsewhere in Studio, and the new addresses. Marking any # of them would tell clients to leave an operation that has no successor. # Resources superseded in their own right are not listed: whether they are # marked is decided with their successors, not here. OTHER_RESOURCE_PATHS = ( "/api/contentstore/v1/videos/{course_id}", - LEGACY_DOWNLOAD_PATH, "/api/contentstore/v0/videos/encodings/{course_id}", "/api/contentstore/v0/videos/features", "/api/contentstore/v0/videos/images/{course_id}/{edx_video_id}", "/api/contentstore/v2/home/courses", "/api/contentstore/v2/downstreams/{usage_key_string}", USAGE_PATH, + ARCHIVE_PATH, ) def marked_schema(self): - """Run the post-processing hook over the superseded path and those of other resources.""" + """Run the post-processing hook over the superseded paths and those of other resources.""" paths = { LEGACY_USAGE_PATH: {"get": {}}, + LEGACY_DOWNLOAD_PATH: {"put": {}}, } paths.update({path: {"get": {}, "post": {}} for path in self.OTHER_RESOURCE_PATHS}) return cms_mark_migrated_paths({"paths": paths}, None, None, True) - def test_post_processing_marks_the_superseded_operation_deprecated(self): + def test_post_processing_marks_the_two_superseded_operations_deprecated(self): paths = self.marked_schema()["paths"] assert paths[LEGACY_USAGE_PATH] == {"get": {"deprecated": True}} + assert paths[LEGACY_DOWNLOAD_PATH] == {"put": {"deprecated": True}} def test_post_processing_leaves_other_resources_alone(self): paths = self.marked_schema()["paths"] @@ -351,20 +359,21 @@ def test_post_processing_leaves_other_resources_alone(self): def test_post_processing_leaves_other_members_of_a_path_alone(self): result = cms_mark_migrated_paths( - {"paths": {LEGACY_USAGE_PATH: {"get": {}, "parameters": []}}}, None, None, True, + {"paths": {LEGACY_DOWNLOAD_PATH: {"put": {}, "parameters": []}}}, None, None, True, ) - assert result["paths"][LEGACY_USAGE_PATH] == {"get": {"deprecated": True}, "parameters": []} + assert result["paths"][LEGACY_DOWNLOAD_PATH] == {"put": {"deprecated": True}, "parameters": []} @ddt.ddt class CmsSchemaDocumentTest(TestCase): """The published Studio document, generated with the production schema settings.""" - def test_the_new_operation_is_published_at_its_full_address(self): + def test_both_new_operations_are_published_at_their_full_address(self): document = production_document() assert [method for method in document["paths"][USAGE_PATH] if method != "parameters"] == ["get"] + assert [method for method in document["paths"][ARCHIVE_PATH] if method != "parameters"] == ["post"] def test_only_the_studio_apis_are_published(self): paths = list(production_document()["paths"]) @@ -381,17 +390,19 @@ def test_no_other_address_of_these_resources_is_published(self): if path.startswith("/api/authoring/") and {"video_usages", "video_archives"} & set(path.split("/")) } - assert published == {USAGE_PATH} + assert published == {USAGE_PATH, ARCHIVE_PATH} - def test_the_published_address_resolves_to_the_new_view(self): + def test_the_published_addresses_resolve_to_the_new_views(self): usage = resolve(USAGE_PATH.format(course_key="course-v1:a+b+c", edx_video_id="v1")) + archive = resolve(ARCHIVE_PATH.format(course_key="course-v1:a+b+c")) - assert usage.func.cls == CourseVideoUsageViewSet + assert (usage.func.cls, archive.func.cls) == (CourseVideoUsageViewSet, CourseVideoArchiveView) def test_operation_ids(self): document = production_document() assert document["paths"][USAGE_PATH]["get"]["operationId"] == "v2_courses_video_usages_retrieve" + assert document["paths"][ARCHIVE_PATH]["post"]["operationId"] == "v2_courses_video_archives_create" assert document["paths"][LEGACY_USAGE_PATH]["get"]["operationId"] == "v1_videos_usage_retrieve" assert document["paths"][LEGACY_DOWNLOAD_PATH]["put"]["operationId"] == "v1_videos_download_update" @@ -417,18 +428,18 @@ def test_no_authoring_operation_is_given_the_id_of_another(self): (path, method) for path, method, __ in operations(production_document()) ) - # Published operations nothing supersedes: the neighbours of the - # superseded one, other Studio resources, and the new address. Resources + # Published operations nothing supersedes: the neighbours of the two + # superseded ones, other Studio resources, and the new addresses. Resources # superseded in their own right are not listed. UNMARKED_OPERATIONS = ( ("/api/contentstore/v1/videos/{course_id}", "get"), - (LEGACY_DOWNLOAD_PATH, "put"), ("/api/contentstore/v0/videos/encodings/{course_id}", "get"), ("/api/contentstore/v0/videos/features", "get"), ("/api/contentstore/v0/videos/images/{course_id}/{edx_video_id}", "post"), ("/api/contentstore/v2/home/courses", "get"), ("/api/contentstore/v2/downstreams/{usage_key_string}", "get"), (USAGE_PATH, "get"), + (ARCHIVE_PATH, "post"), ) @staticmethod @@ -436,10 +447,11 @@ def marks(document, path, method): operation = document["paths"][path][method] return operation.get("deprecated"), operation.get("x-internal") - def test_the_legacy_usage_operation_is_deprecated(self): + def test_the_two_legacy_operations_are_deprecated(self): document = production_document() assert self.marks(document, LEGACY_USAGE_PATH, "get") == (True, None) + assert self.marks(document, LEGACY_DOWNLOAD_PATH, "put") == (True, None) def test_other_operations_are_left_unmarked(self): document = production_document() @@ -457,6 +469,13 @@ def test_other_operations_are_left_unmarked(self): "sets token_type=jwt), or the session cookie of a signed-in Studio user whose account is " "active. OAuth2 access tokens sent as 'Bearer ' are not accepted." )), + (ARCHIVE_PATH, "post", ( + "The request carries no accepted credentials. Send a JSON Web Token in the Authorization " + "header as 'JWT ' (the OAuth2 access token endpoint issues one when the token request " + "sets token_type=jwt), or the session cookie of a signed-in Studio user whose account is " + "active, together with the CSRF token in the X-CSRFToken header. OAuth2 access tokens sent " + "as 'Bearer ' are not accepted." + )), ) @ddt.unpack def test_the_accepted_credentials_are_named(self, path, method, description): @@ -466,6 +485,15 @@ def test_the_accepted_credentials_are_named(self, path, method, description): @ddt.data( (USAGE_PATH, "get", "403", "The requester does not have Studio access to the course."), + (ARCHIVE_PATH, "post", "403", ( + "The requester does not have Studio access to the course, or a request made with a session " + "did not send the CSRF token." + )), + (ARCHIVE_PATH, "post", "406", ( + "The Accept header does not admit application/json. Errors are sent as JSON, so a request " + "for the archive sends 'Accept: application/zip, application/json', or no Accept header. " + "Nothing is fetched." + )), ) @ddt.unpack def test_each_refusal_names_its_reasons(self, path, method, code, description): @@ -473,10 +501,11 @@ def test_each_refusal_names_its_reasons(self, path, method, code, description): assert operation["responses"][code]["description"] == description - def test_the_new_operation_is_tagged_for_the_sdk(self): + def test_both_new_operations_are_tagged_for_the_sdk(self): document = production_document() assert document["paths"][USAGE_PATH]["get"]["tags"] == ["openedx-platform-sdk"] + assert document["paths"][ARCHIVE_PATH]["post"]["tags"] == ["openedx-platform-sdk"] def component(self, document, reference): return document["components"]["schemas"][reference["$ref"].rsplit("/", 1)[-1]] @@ -494,8 +523,26 @@ def test_the_usage_response_is_declared(self): name: field.help_text for name, field in fields.items() } + def test_the_archive_request_body_is_declared(self): + document = production_document() + request_body = document["paths"][ARCHIVE_PATH]["post"]["requestBody"] + + body = self.component(document, request_body["content"]["application/json"]["schema"]) + entry = self.component(document, body["properties"]["files"]["items"]) + assert list(request_body["content"]) == ["application/json"] + assert request_body["required"] is True + assert body["required"] == ["files"] + assert sorted(entry["required"]) == ["name", "url"] + + def test_the_archive_response_is_a_binary_zip(self): + response = production_document()["paths"][ARCHIVE_PATH]["post"]["responses"]["200"] + + assert response["content"] == {"application/zip": {"schema": {"type": "string", "format": "binary"}}} + assert "_videos.zip" in response["description"] + @ddt.data( (USAGE_PATH, "get", ["401", "403", "404"]), + (ARCHIVE_PATH, "post", ["400", "401", "403", "404", "406", "415", "429"]), ) @ddt.unpack def test_every_error_response_is_the_error_envelope(self, path, method, codes): @@ -543,3 +590,19 @@ def test_no_server_url_carries_an_api_prefix(self, module): assert [server["description"] for server in servers] == ["Public", "Local"] assert [server for server in servers if "/api/" in server["url"]] == [] + + +class StudioApiDocsTest(TestCase): + """The Studio API documentation site, which documents each view from what the view declares.""" + + ARCHIVE_DOCS_PATH = "/authoring/v2/courses/{course_key}/video_archives/" + + # The site reads a view's serializer_class as both its request and its + # response body. The archive view answers with a binary archive, so it + # declares none, and the site shows no body for it rather than a JSON one. + def test_the_archive_operation_is_given_no_json_body(self): + response = self.client.get(reverse("apidocs-data", kwargs={"format": ".json"})) + + operation = json.loads(response.content)["paths"][self.ARCHIVE_DOCS_PATH]["post"] + assert [parameter for parameter in operation["parameters"] if parameter["in"] == "body"] == [] + assert {code: body for code, body in operation["responses"].items() if "schema" in body} == {} diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/video_archives.py b/cms/djangoapps/contentstore/rest_api/v2/views/video_archives.py new file mode 100644 index 000000000000..02051a029497 --- /dev/null +++ b/cms/djangoapps/contentstore/rest_api/v2/views/video_archives.py @@ -0,0 +1,127 @@ +"""API Views for zip archives of a course's videos.""" + +from drf_spectacular.types import OpenApiTypes +from drf_spectacular.utils import OpenApiExample, OpenApiResponse, extend_schema +from edx_rest_framework_extensions.errors import ErrorResponseSerializer +from edx_rest_framework_extensions.mixins import StandardizedErrorMixin +from rest_framework.parsers import JSONParser +from rest_framework.permissions import IsAuthenticated +from rest_framework.views import APIView + +from cms.djangoapps.contentstore.rest_api.v1.views.videos import VideoDownloadThrottle +from cms.djangoapps.contentstore.rest_api.v2.serializers.video_archives import VideoArchiveRequestSerializer +from cms.djangoapps.contentstore.rest_api.v2.video_management_service import ( + create_course_video_archive, + ensure_course_exists, +) +from cms.djangoapps.contentstore.rest_api.v2.views.video_schema import ( + COURSE_KEY_PARAMETER, + COURSE_NOT_FOUND_RESPONSE, + forbidden_response, + unauthenticated_response, +) +from cms.djangoapps.contentstore.views.permissions import HasFullCourseKey, HasStudioReadAccess + +_REQUEST_EXAMPLE = OpenApiExample( + "Two videos", + value={ + "files": [ + {"url": "https://video-store.example.com/course-videos/lecture-01.mp4", "name": "Lecture 1.mp4"}, + {"url": "https://video-store.example.com/course-videos/lecture-02.mp4", "name": "Lecture 2"}, + ] + }, + request_only=True, +) + +_RESPONSE_ARCHIVE = OpenApiResponse( + response=OpenApiTypes.BINARY, + description=( + "A zip archive holding one entry per requested video, in request order, sent as an " + "attachment named '_videos.zip'. The archive is streamed while the videos " + "are fetched; if fetching a video fails part way, the transfer stops and the archive " + "received is incomplete." + ), +) +_RESPONSE_BAD_REQUEST = OpenApiResponse( + response=ErrorResponseSerializer, + description=( + "The request body is malformed, or names an address that is not one of the course's " + "video files. Nothing is fetched." + ), +) +_RESPONSE_NOT_ACCEPTABLE = OpenApiResponse( + response=ErrorResponseSerializer, + description=( + "The Accept header does not admit application/json. Errors are sent as JSON, so a request " + "for the archive sends 'Accept: application/zip, application/json', or no Accept header. " + "Nothing is fetched." + ), +) +_RESPONSE_UNSUPPORTED_MEDIA_TYPE = OpenApiResponse( + response=ErrorResponseSerializer, + description="The request body is not JSON. Nothing is fetched.", +) +_RESPONSE_THROTTLED = OpenApiResponse( + response=ErrorResponseSerializer, + description=( + "The requester has made too many archive requests recently. The Retry-After header gives " + "the number of seconds to wait." + ), +) + + +@extend_schema(tags=["openedx-platform-sdk"]) +class CourseVideoArchiveView(StandardizedErrorMixin, APIView): + """ + Build a zip archive of chosen videos of a course. + + The archive is assembled while it is sent and is never stored, so there is + no collection to list and no member to fetch or delete afterwards; that is + why this is a single POST handler rather than a ViewSet. Each requested + address must be one of the course's own encoded-video URLs and is checked + before any video is fetched, so the server only ever fetches addresses the + course already holds. + + The body is accepted as JSON only: a list of objects has no form encoding + that both the published schema and the parser agree on. + + Callers need Studio read access to the course. Requests are rate-limited + per user with the throttle of the v1 video download endpoint, so both + endpoints draw on one budget. + """ + + permission_classes = (IsAuthenticated, HasFullCourseKey, HasStudioReadAccess) + parser_classes = (JSONParser,) + throttle_classes = (VideoDownloadThrottle,) + + @extend_schema( + summary="Download chosen videos of a course as one zip archive", + description=( + "Fetches the requested videos of the course and streams them back as a single zip " + "archive. The archive is built for this request only and is not stored. Every " + "address must be one of the course's encoded video files, as the course's video " + "listing reports them; a request naming any other address is refused as a whole " + "before anything is fetched. Requests are rate-limited per user." + ), + parameters=[COURSE_KEY_PARAMETER], + request=VideoArchiveRequestSerializer, + examples=[_REQUEST_EXAMPLE], + responses={ + (200, "application/zip"): _RESPONSE_ARCHIVE, + 400: _RESPONSE_BAD_REQUEST, + 401: unauthenticated_response(requires_csrf_token=True), + 403: forbidden_response(requires_csrf_token=True), + 404: COURSE_NOT_FOUND_RESPONSE, + 406: _RESPONSE_NOT_ACCEPTABLE, + 415: _RESPONSE_UNSUPPORTED_MEDIA_TYPE, + 429: _RESPONSE_THROTTLED, + }, + ) + def post(self, request, course_key): + """Stream an archive of the requested videos of the course.""" + ensure_course_exists(course_key) + # The view declares no serializer_class: documentation generators read + # it as the response body as well, and the response is a binary archive. + body = VideoArchiveRequestSerializer(data=request.data) + body.is_valid(raise_exception=True) + return create_course_video_archive(course_key, body.validated_data["files"]) diff --git a/cms/lib/spectacular.py b/cms/lib/spectacular.py index 22ae63b60c24..ba803609c93c 100644 --- a/cms/lib/spectacular.py +++ b/cms/lib/spectacular.py @@ -7,6 +7,8 @@ SUPERSEDED_PATH_PREFIXES = ( # → /api/authoring/v2/courses/{course_key}/video_usages/{edx_video_id}/ "/api/contentstore/v1/videos/{course_id}/{edx_video_id}/usage", + # → /api/authoring/v2/courses/{course_key}/video_archives/ + "/api/contentstore/v1/videos/{course_id}/download", ) From cd38a23be0c759f8689a5df0e55adcf848db5476 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Mon, 5 Oct 2026 01:06:03 +0500 Subject: [PATCH 4/7] test: use the standard library UTC in the video archive tests The platform's type check runs mypy over this module, and pytz ships no type stubs, so its import failed `make check-types`. datetime.UTC gives the same aware timestamp without the third-party import. Part of #39071. --- .../rest_api/v2/views/tests/test_video_archives.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_archives.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_archives.py index 13d25ed8df50..cf743ace2b08 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_archives.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_archives.py @@ -4,13 +4,12 @@ import io import json import zipfile -from datetime import datetime +from datetime import UTC, datetime from unittest.mock import MagicMock, patch from urllib.parse import urlencode import ddt import pytest -import pytz import requests from django.core.signals import got_request_exception from django.urls import Resolver404, resolve, reverse @@ -119,7 +118,7 @@ def create_course_video(self, edx_video_id, course_key, encodings, also_in=()): "duration": 42.0, "status": "file_complete", "courses": [str(key) for key in (course_key, *also_in)], - "created": datetime.now(pytz.utc), + "created": datetime.now(UTC), "encoded_videos": [ {"profile": profile, "url": url, "file_size": 1600, "bitrate": 100} for profile, url in encodings From 050c98ee2875f2b458d87e66176f44384f99ec82 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Wed, 14 Oct 2026 11:55:21 +0500 Subject: [PATCH 5/7] test: follow master's schema hook and pin the viewset mount's actions The two superseded video operations now live in SUPERSEDED_PATHS and are marked by cms_mark_superseded_paths, so the hook tests exercise that hook and the settings test requires all three registered hooks rather than two of them. The class a viewset address resolves to says nothing about which actions that mount wired up, so the method-to-action map is pinned with the shared routed_actions helper. Co-Authored-By: Claude Opus 5 --- .../tests/test_video_management_contract.py | 32 +++++++++++-------- .../v2/views/tests/test_video_usages.py | 5 ++- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py index baf0366c3354..e85a05a078b8 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py @@ -25,9 +25,9 @@ from cms.djangoapps.contentstore.rest_api.v2.serializers.video_usages import VideoUsageLocationSerializer from cms.djangoapps.contentstore.rest_api.v2.views.video_archives import CourseVideoArchiveView from cms.djangoapps.contentstore.rest_api.v2.views.video_usages import CourseVideoUsageViewSet -from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from cms.djangoapps.contentstore.tests.utils import CourseTestCase, routed_actions from cms.djangoapps.contentstore.views.permissions import HasFullCourseKey, HasStudioReadAccess -from cms.lib.spectacular import SUPERSEDED_PATH_PREFIXES, cms_api_filter, cms_mark_migrated_paths +from cms.lib.spectacular import SUPERSEDED_PATHS, cms_api_filter, cms_mark_superseded_paths from common.djangoapps.student.roles import ( CourseInstructorRole, CourseLimitedStaffRole, @@ -42,6 +42,7 @@ LEGACY_DOWNLOAD_PATH = "/api/contentstore/v1/videos/{course_id}/download" ENUM_POSTPROCESSING_HOOK = "drf_spectacular.hooks.postprocess_schema_enums" +SUPERSEDED_PATHS_HOOK = "cms.lib.spectacular.cms_mark_superseded_paths" MIGRATED_PATHS_HOOK = "cms.lib.spectacular.cms_mark_migrated_paths" PRODUCTION_SETTINGS = "cms.envs.production" @@ -314,10 +315,11 @@ def test_filter_admits_the_course_discussions_switch_and_nothing_else_under_cour "/api/courses/{course_key_string}/bulk_enable_disable_discussions", ] + # Listed as exact paths rather than prefixes: the operations beside them + # under /api/contentstore/v1/videos/ have no successor, and a prefix would + # also reach any sibling later added below one of these two addresses. def test_the_superseded_paths_include_the_two_video_operations(self): - assert [ - prefix for prefix in SUPERSEDED_PATH_PREFIXES if prefix in (LEGACY_USAGE_PATH, LEGACY_DOWNLOAD_PATH) - ] == [LEGACY_USAGE_PATH, LEGACY_DOWNLOAD_PATH] + assert {LEGACY_USAGE_PATH, LEGACY_DOWNLOAD_PATH} <= set(SUPERSEDED_PATHS) # Operations of other resources that nothing supersedes, next to the two # superseded ones or elsewhere in Studio, and the new addresses. Marking any @@ -342,7 +344,7 @@ def marked_schema(self): LEGACY_DOWNLOAD_PATH: {"put": {}}, } paths.update({path: {"get": {}, "post": {}} for path in self.OTHER_RESOURCE_PATHS}) - return cms_mark_migrated_paths({"paths": paths}, None, None, True) + return cms_mark_superseded_paths({"paths": paths}, None, None, True) def test_post_processing_marks_the_two_superseded_operations_deprecated(self): paths = self.marked_schema()["paths"] @@ -358,7 +360,7 @@ def test_post_processing_leaves_other_resources_alone(self): } def test_post_processing_leaves_other_members_of_a_path_alone(self): - result = cms_mark_migrated_paths( + result = cms_mark_superseded_paths( {"paths": {LEGACY_DOWNLOAD_PATH: {"put": {}, "parameters": []}}}, None, None, True, ) @@ -392,11 +394,16 @@ def test_no_other_address_of_these_resources_is_published(self): assert published == {USAGE_PATH, ARCHIVE_PATH} + # The usage address is a viewset mount, whose handlers come from the action + # map it was registered with rather than from the class, so that map is + # pinned too. The archive address is a plain view: its class is the whole + # contract. def test_the_published_addresses_resolve_to_the_new_views(self): usage = resolve(USAGE_PATH.format(course_key="course-v1:a+b+c", edx_video_id="v1")) archive = resolve(ARCHIVE_PATH.format(course_key="course-v1:a+b+c")) assert (usage.func.cls, archive.func.cls) == (CourseVideoUsageViewSet, CourseVideoArchiveView) + assert routed_actions(usage.func) == {"get": "retrieve"} def test_operation_ids(self): document = production_document() @@ -555,7 +562,7 @@ def test_every_error_response_is_the_error_envelope(self, path, method, codes): } def test_enum_components_survive_the_registered_post_processing(self): - without_enum_hook = generate_document(postprocessing_hooks=[MIGRATED_PATHS_HOOK]) + without_enum_hook = generate_document(postprocessing_hooks=[SUPERSEDED_PATHS_HOOK]) assert not [name for name in without_enum_hook["components"]["schemas"] if name.endswith("Enum")] assert "ContentTypeEnum" in production_document()["components"]["schemas"] @@ -573,13 +580,12 @@ def test_paths_are_published_in_full(self, module): assert configured["SCHEMA_PATH_PREFIX"] == r"/api/(contentstore|authoring)" @ddt.data(PRODUCTION_SETTINGS, DEVSTACK_SETTINGS) - def test_the_post_processing_hooks_keep_the_enum_hook(self, module): + # These modules replace SPECTACULAR_SETTINGS wholesale, so a hook left out + # here is silently disabled for the deployment that uses them. + def test_the_post_processing_hooks_are_all_registered(self, module): hooks = schema_settings(module)["POSTPROCESSING_HOOKS"] - assert [hook for hook in hooks if hook in (ENUM_POSTPROCESSING_HOOK, MIGRATED_PATHS_HOOK)] == [ - ENUM_POSTPROCESSING_HOOK, - MIGRATED_PATHS_HOOK, - ] + assert hooks == [ENUM_POSTPROCESSING_HOOK, SUPERSEDED_PATHS_HOOK, MIGRATED_PATHS_HOOK] # The Public server is AUTHORING_API_URL as configured. Whether a gateway # behind it serves the full paths is not decided by these settings and is diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_usages.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_usages.py index 1d6714ba88d1..40d597af3640 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_usages.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_usages.py @@ -21,7 +21,7 @@ from cms.djangoapps.contentstore.rest_api.v1.views.unknown_route import UnknownRouteView from cms.djangoapps.contentstore.rest_api.v2.views.video_usages import CourseVideoUsageViewSet -from cms.djangoapps.contentstore.tests.utils import CourseTestCase +from cms.djangoapps.contentstore.tests.utils import CourseTestCase, routed_actions from common.djangoapps.student.roles import ( CourseInstructorRole, CourseLimitedStaffRole, @@ -754,10 +754,13 @@ class VideoUsageUrlContractTest(VideoUsageTestBase): def test_conforming_address(self): assert self.url("abc-123") == f"/api/authoring/v2/courses/{self.course.id}/video_usages/abc-123/" + # The class alone says nothing about which actions this mount wires up, so + # the method-to-action map is pinned as well. def test_the_address_resolves_to_the_viewset(self): match = resolve(self.url("abc-123")) assert match.func.cls is CourseVideoUsageViewSet + assert routed_actions(match.func) == {"get": "retrieve"} assert match.kwargs == {"course_key": self.course.id, "edx_video_id": "abc-123"} def test_the_legacy_address_is_unchanged(self): From ec62c3a6dd3912f625120734aa2a104796e6c85d Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Wed, 14 Oct 2026 12:11:46 +0500 Subject: [PATCH 6/7] test: list the two video paths as superseded and read the new docs document The schema-hook suite pins the whole SUPERSEDED_PATHS set, so the two video addresses are listed there with the youtube pair. The service-wide documentation site is now generated by drf-spectacular: its schema view takes the format without a leading dot, it publishes paths in full rather than stripped of their API prefix, and an operation carries requestBody and per-status content instead of body parameters and a bare schema. The archive assertions read that shape, and check the operation is offered only as a zip. Co-Authored-By: Claude Opus 5 --- .../tests/test_video_management_contract.py | 38 ++++++++++++++----- cms/lib/tests/test_spectacular.py | 6 ++- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py index e85a05a078b8..322ad4d88c86 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_management_contract.py @@ -599,16 +599,34 @@ def test_no_server_url_carries_an_api_prefix(self, module): class StudioApiDocsTest(TestCase): - """The Studio API documentation site, which documents each view from what the view declares.""" + """ + The service-wide documentation site, which documents every endpoint of Studio. + + It applies none of the Authoring API's filtering or prefix stripping, so it + carries both new operations at their full addresses. + """ + + ARCHIVE_DOCS_PATH = "/api/authoring/v2/courses/{course_key}/video_archives/" + USAGE_DOCS_PATH = "/api/authoring/v2/courses/{course_key}/video_usages/{edx_video_id}/" - ARCHIVE_DOCS_PATH = "/authoring/v2/courses/{course_key}/video_archives/" + def document(self): + response = self.client.get(reverse("apidocs-data", kwargs={"format": "json"})) + assert response.status_code == 200 + return json.loads(response.content) - # The site reads a view's serializer_class as both its request and its - # response body. The archive view answers with a binary archive, so it - # declares none, and the site shows no body for it rather than a JSON one. - def test_the_archive_operation_is_given_no_json_body(self): - response = self.client.get(reverse("apidocs-data", kwargs={"format": ".json"})) + def test_both_operations_are_documented_at_their_full_addresses(self): + paths = self.document()["paths"] - operation = json.loads(response.content)["paths"][self.ARCHIVE_DOCS_PATH]["post"] - assert [parameter for parameter in operation["parameters"] if parameter["in"] == "body"] == [] - assert {code: body for code, body in operation["responses"].items() if "schema" in body} == {} + assert list(paths[self.ARCHIVE_DOCS_PATH]) == ["post"] + assert list(paths[self.USAGE_DOCS_PATH]) == ["get"] + + # A documentation generator that infers the response body from the view's + # serializer would describe the archive as JSON. The view declares the + # response itself, so the only representation offered is the archive. + def test_the_archive_response_is_offered_only_as_a_zip(self): + operation = self.document()["paths"][self.ARCHIVE_DOCS_PATH]["post"] + + assert operation["responses"]["200"]["content"] == { + "application/zip": {"schema": {"type": "string", "format": "binary"}} + } + assert list(operation["requestBody"]["content"]) == ["application/json"] diff --git a/cms/lib/tests/test_spectacular.py b/cms/lib/tests/test_spectacular.py index 4581f23a6148..0d847bd7ddb6 100644 --- a/cms/lib/tests/test_spectacular.py +++ b/cms/lib/tests/test_spectacular.py @@ -70,12 +70,16 @@ def test_every_operation_of_a_superseded_path_is_marked(self): for path in SUPERSEDED_PATHS: assert result["paths"][path]["get"]["deprecated"] is True - def test_both_spellings_of_each_superseded_path_are_listed(self): + def test_every_superseded_path_is_listed(self): assert set(SUPERSEDED_PATHS) == { + # The youtube_transcripts routes make the final character optional, + # so each of them is listed in both spellings. "/api/contentstore/v0/youtube_transcripts/{course_id}/chec", "/api/contentstore/v0/youtube_transcripts/{course_id}/check", "/api/contentstore/v0/youtube_transcripts/{course_id}/uploa", "/api/contentstore/v0/youtube_transcripts/{course_id}/upload", + "/api/contentstore/v1/videos/{course_id}/{edx_video_id}/usage", + "/api/contentstore/v1/videos/{course_id}/download", } def test_the_conforming_paths_are_left_unmarked(self): From c13404af2240aed8596d5c22613691a911723225 Mon Sep 17 00:00:00 2001 From: Abdul-Muqadim-Arbisoft Date: Wed, 14 Oct 2026 12:18:31 +0500 Subject: [PATCH 7/7] fix: record that the two legacy video operations are now superseded The video settings suite asserted that the usage and download operations beside the legacy videos listing carry no deprecation mark, which held while neither had a successor. Both have one now, so the suite checks that they are marked and keeps the assertion that the listing is not -- that is what catches a change marking the whole legacy videos prefix instead of the two exact paths. The archive endpoint draws on the video-download throttle, so the rate limit setting names both endpoints and says they share one budget. Co-Authored-By: Claude Opus 5 --- .../rest_api/v2/views/tests/test_video_settings.py | 11 +++++++++-- cms/envs/common.py | 8 +++++--- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_settings.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_settings.py index 41aef38e3e5a..ed575012a281 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_settings.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_video_settings.py @@ -458,13 +458,20 @@ def test_every_error_references_the_shared_envelope(self): "$ref": "#/components/schemas/ErrorResponse" }, code - def test_the_legacy_video_operations_are_unchanged(self): + def test_the_legacy_videos_listing_is_unchanged(self): operation = self.schema["paths"][LEGACY_VIDEOS_PATH]["get"] assert operation["operationId"] == "v1_videos_retrieve" assert "deprecated" not in operation + + # The usage and download operations beside the listing have conforming + # successors of their own, so they carry the mark. The listing does not, + # which is what the assertion above guards: a change that marked the whole + # legacy videos prefix rather than the two exact paths would sweep it up. + def test_the_legacy_siblings_of_the_listing_are_deprecated(self): for path, view in self._legacy_sibling_paths(): for operation in self.schema["paths"][path].values(): - assert "deprecated" not in operation, (path, view) + if isinstance(operation, dict): + assert operation["deprecated"] is True, (path, view) def _legacy_sibling_paths(self): """Return the legacy usage and download paths published beside the legacy videos path.""" diff --git a/cms/envs/common.py b/cms/envs/common.py index 87ee13f3e0a6..06ac9c6c8823 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -667,9 +667,11 @@ def make_lms_template_path(settings): # .. setting_name: VIDEO_DOWNLOAD_RATE_LIMIT # .. setting_default: '12/hour' # .. setting_description: Per-user rate limit applied to the Studio -# video-download endpoint -# (``PUT /api/contentstore/v1/videos/{course_id}/download``). Bounds how -# often a single course author can initiate a multi-video zip download. +# video-download endpoints +# (``PUT /api/contentstore/v1/videos/{course_id}/download`` and +# ``POST /api/authoring/v2/courses/{course_key}/video_archives/``, which +# share one budget). Bounds how often a single course author can initiate a +# multi-video zip download. # Rate format: DRF ``UserRateThrottle`` rate string. See # https://www.django-rest-framework.org/api-guide/throttling/#setting-the-throttling-policy VIDEO_DOWNLOAD_RATE_LIMIT = '12/hour'