Ccx notes

.github/workflows/unit-tests.yml

@@ -8,6 +8,7 @@ on:
   push:
     branches:
       - master
+      - release/*
 
 concurrency:
   # We only need to be running tests for the latest commit on each PR

.gitignore

@@ -138,6 +138,9 @@ database.sqlite
 courseware/static/js/mathjax/*
 flushdb.sh
 build
+# Vendored pdf.js needs a build/ subdir whose name happens to collide with the
+# generic 'build' ignore above.
+!/common/static/js/vendor/pdfjs/build/
 /src/
 \#*\#
 .env/

README.rst

@@ -230,6 +230,23 @@ running at the given ports.
      - localhost:1997
      - ACCOUNT_MICROFRONTEND_URL
 
+Security Deployment Requirements
+********************************
+
+Some platform features require a **shared** Django cache backend (Redis or
+Memcached) to function correctly across multiple LMS nodes:
+
+* **LTI Provider** — OAuth nonce replay protection stores seen nonces in the
+  Django ``default`` cache. A per-process backend (e.g. ``LocMemCache``) will
+  not detect replays that arrive on a different node. See
+  `lms/djangoapps/lti_provider/README.rst`_ for details.
+
+Tutor-based deployments satisfy this requirement automatically. For bare-metal
+or custom deployments, verify that ``CACHES['default']`` points at a shared
+Redis or Memcached instance before enabling these features.
+
+.. _lms/djangoapps/lti_provider/README.rst: lms/djangoapps/lti_provider/README.rst
+
 License
 *******
 

cms/djangoapps/contentstore/asset_storage_handlers.py

@@ -26,19 +26,19 @@
 from openedx_filters.content_authoring.filters import LMSPageURLRequested
 from pymongo import ASCENDING, DESCENDING
 
+from common.djangoapps.student.roles import enable_authz_course_authoring
 from common.djangoapps.util.date_utils import get_default_time_display
 from common.djangoapps.util.json_request import JsonResponse
 from openedx.core.djangoapps.authz.constants import LegacyAuthoringPermission
 from openedx.core.djangoapps.authz.decorators import user_has_course_permission
 from openedx.core.djangoapps.contentserver.caching import del_cached_content
 from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers
 from openedx.core.djangoapps.user_api.models import UserPreference
-from openedx.core.toggles import enable_authz_course_authoring
-from xmodule.contentstore.content import StaticContent  # lint-amnesty, pylint: disable=wrong-import-order
-from xmodule.contentstore.django import contentstore  # lint-amnesty, pylint: disable=wrong-import-order
-from xmodule.exceptions import NotFoundError  # lint-amnesty, pylint: disable=wrong-import-order
-from xmodule.modulestore.django import modulestore  # lint-amnesty, pylint: disable=wrong-import-order
-from xmodule.modulestore.exceptions import ItemNotFoundError  # lint-amnesty, pylint: disable=wrong-import-order
+from xmodule.contentstore.content import StaticContent  # pylint: disable=wrong-import-order
+from xmodule.contentstore.django import contentstore  # pylint: disable=wrong-import-order
+from xmodule.exceptions import NotFoundError  # pylint: disable=wrong-import-order
+from xmodule.modulestore.django import modulestore  # pylint: disable=wrong-import-order
+from xmodule.modulestore.exceptions import ItemNotFoundError  # pylint: disable=wrong-import-order
 
 from .exceptions import AssetNotFoundException, AssetSizeTooLargeException
 from .utils import get_files_uploads_url, get_response_format, request_response_format_is_json

cms/djangoapps/contentstore/rest_api/v1/serializers/course_waffle_flags.py

@@ -5,7 +5,7 @@
 from rest_framework import serializers
 
 from cms.djangoapps.contentstore import toggles
-from openedx.core import toggles as core_toggles
+from common.djangoapps.student.roles import enable_authz_course_authoring
 
 
 class CourseWaffleFlagsSerializer(serializers.Serializer):
@@ -221,4 +221,4 @@ def get_enable_authz_course_authoring(self, obj):
         Method to get the authz.enable_course_authoring waffle flag
         """
         course_key = self.get_course_key()
-        return core_toggles.enable_authz_course_authoring(course_key)
+        return enable_authz_course_authoring(course_key)

cms/djangoapps/contentstore/rest_api/v1/views/tests/test_videos.py

@@ -1,20 +1,25 @@
 """
 Unit tests for course settings views.
 """
-from unittest.mock import patch
+from datetime import datetime
+from unittest.mock import MagicMock, patch
 
 import ddt
+import pytz
 from django.conf import settings
 from django.contrib.staticfiles.storage import staticfiles_storage
 from django.urls import reverse
 from edx_toggles.toggles import WaffleSwitch
 from edx_toggles.toggles.testutils import override_waffle_switch
 from edxval.api import (
+    create_profile,
+    create_video,
     get_3rd_party_transcription_plans,
     get_transcript_credentials_state_for_org,
     get_transcript_preferences,
 )
 from rest_framework import status
+from rest_framework.test import APIClient
 
 from cms.djangoapps.contentstore.tests.utils import CourseTestCase
 from cms.djangoapps.contentstore.utils import reverse_course_url
@@ -135,3 +140,106 @@ def test_VideoTranscriptEnabledFlag_enabled(self):
             response = self.client.get(self.url)
             self.assertIn("is_ai_translations_enabled", response.data)  # noqa: PT009
             self.assertTrue(response.data["is_ai_translations_enabled"])  # noqa: PT009
+
+
+class VideoDownloadViewTest(CourseTestCase):
+    """
+    Tests for VideoDownloadView.
+
+    The download endpoint fetches each requested ``files[].url`` server-side and
+    returns the bytes inside a zip. Those URLs must therefore be restricted to
+    the course's own video URLs, otherwise the endpoint is an SSRF primitive
+    (see GHSA-fpf9-9rpr-jvrx).
+    """
+
+    ALLOWED_URL = "http://example.com/profile1/test.mp4"
+    # An internal address an attacker might try to reach via SSRF.
+    SSRF_URL = "http://169.254.169.254/latest/meta-data/"
+
+    def setUp(self):
+        super().setUp()
+        # reverse() with only course_id resolves to the download route (the
+        # usage route with the same name additionally requires edx_video_id).
+        self.url = reverse(
+            "cms.djangoapps.contentstore:v1:video_usage",
+            kwargs={"course_id": self.course.id},
+        )
+        self.api_client = APIClient()
+        self.api_client.force_authenticate(user=self.user)
+        create_profile("profile1")
+        create_video({
+            "edx_video_id": "test-video",
+            "client_video_id": "test.mp4",
+            "duration": 42.0,
+            "status": "file_complete",
+            "courses": [str(self.course.id)],
+            "created": datetime.now(pytz.utc),
+            "encoded_videos": [
+                {
+                    "profile": "profile1",
+                    "url": self.ALLOWED_URL,
+                    "file_size": 1600,
+                    "bitrate": 100,
+                },
+            ],
+        })
+
+    @patch("cms.djangoapps.contentstore.video_storage_handlers.requests.get")
+    def test_download_allowed_url(self, mock_get):
+        """A URL that belongs to the course's videos is fetched and zipped."""
+        mock_get.return_value = MagicMock(
+            content=b"video-bytes",
+            headers={"Content-Type": "video/mp4"},
+        )
+        response = self.api_client.put(
+            self.url,
+            data={"files": [{"url": self.ALLOWED_URL, "name": "test.mp4"}]},
+            format="json",
+        )
+        self.assertEqual(response.status_code, status.HTTP_200_OK)  # noqa: PT009
+        mock_get.assert_called_once_with(self.ALLOWED_URL, allow_redirects=True)
+
+    @patch("cms.djangoapps.contentstore.video_storage_handlers.requests.get")
+    def test_rejects_url_not_belonging_to_course(self, mock_get):
+        """
+        A URL that is not one of the course's video URLs is rejected before any
+        server-side request is made (SSRF protection).
+        """
+        response = self.api_client.put(
+            self.url,
+            data={"files": [{"url": self.SSRF_URL, "name": "evil.txt"}]},
+            format="json",
+        )
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)  # noqa: PT009
+        mock_get.assert_not_called()
+
+    @patch("cms.djangoapps.contentstore.video_storage_handlers.requests.get")
+    def test_rejects_when_any_url_is_disallowed(self, mock_get):
+        """
+        A request mixing an allowed URL with a disallowed one is rejected
+        outright, without fetching the allowed URL either.
+        """
+        response = self.api_client.put(
+            self.url,
+            data={"files": [
+                {"url": self.ALLOWED_URL, "name": "test.mp4"},
+                {"url": self.SSRF_URL, "name": "evil.txt"},
+            ]},
+            format="json",
+        )
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)  # noqa: PT009
+        mock_get.assert_not_called()
+
+    @patch("cms.djangoapps.contentstore.video_storage_handlers.requests.get")
+    def test_non_staff_user_denied(self, mock_get):
+        """A user without studio read access cannot reach the fetch path."""
+        __, nonstaff_user = self.create_non_staff_authed_user_client()
+        client = APIClient()
+        client.force_authenticate(user=nonstaff_user)
+        response = client.put(
+            self.url,
+            data={"files": [{"url": self.ALLOWED_URL, "name": "test.mp4"}]},
+            format="json",
+        )
+        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN)  # noqa: PT009
+        mock_get.assert_not_called()

cms/djangoapps/contentstore/rest_api/v2/serializers/home.py

@@ -30,7 +30,9 @@ class CourseCommonSerializerV2(serializers.Serializer):
     lms_link = serializers.SerializerMethodField()
     cms_link = serializers.SerializerMethodField()
     number = serializers.CharField()
+    display_number = serializers.CharField(source='display_number_with_default')
     org = serializers.CharField()
+    display_org = serializers.CharField(source='display_org_with_default')
     rerun_link = serializers.SerializerMethodField()
     run = serializers.CharField(source='id.run')
     url = serializers.SerializerMethodField()

cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py

@@ -57,8 +57,10 @@ def test_home_page_response(self):
                     ("display_name", self.course.display_name),
                     ("lms_link", f'{settings.LMS_ROOT_URL}/courses/{course_id}/jump_to/{self.course.location}'),
                     ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.course.id)}'),
-                    ("number", self.course.number),
-                    ("org", self.course.org),
+                    ("number", self.active_course.number),
+                    ("display_number", self.active_course.display_number_with_default),
+                    ("org", self.active_course.org),
+                    ("display_org", self.active_course.display_org_with_default),
                     ("rerun_link", f'/course_rerun/{course_id}'),
                     ("run", self.course.id.run),
                     ("url", f'/course/{course_id}'),
@@ -76,7 +78,9 @@ def test_home_page_response(self):
                         f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}',
                     ),
                     ("number", self.archived_course.number),
+                    ("display_number", self.archived_course.display_number_with_default),
                     ("org", self.archived_course.org),
+                    ("display_org", self.archived_course.display_org_with_default),
                     ("rerun_link", f'/course_rerun/{str(self.archived_course.id)}'),
                     ("run", self.archived_course.id.run),
                     ("url", f'/course/{str(self.archived_course.id)}'),
@@ -110,8 +114,10 @@ def test_active_only_query_if_passed(self):
             ("display_name", self.course.display_name),
             ("lms_link", f'{settings.LMS_ROOT_URL}/courses/{str(self.course.id)}/jump_to/{self.course.location}'),
             ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.course.id)}'),
-            ("number", self.course.number),
-            ("org", self.course.org),
+            ("number", self.active_course.number),
+            ("display_number", self.active_course.display_number_with_default),
+            ("org", self.active_course.org),
+            ("display_org", self.active_course.display_org_with_default),
             ("rerun_link", f'/course_rerun/{str(self.course.id)}'),
             ("run", self.course.id.run),
             ("url", f'/course/{str(self.course.id)}'),
@@ -141,7 +147,9 @@ def test_archived_only_query_if_passed(self):
             ),
             ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'),
             ("number", self.archived_course.number),
+            ("display_number", self.archived_course.display_number_with_default),
             ("org", self.archived_course.org),
+            ("display_org", self.archived_course.display_org_with_default),
             ("rerun_link", f'/course_rerun/{str(self.archived_course.id)}'),
             ("run", self.archived_course.id.run),
             ("url", f'/course/{str(self.archived_course.id)}'),
@@ -171,7 +179,9 @@ def test_search_query_if_passed(self):
             ),
             ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'),
             ("number", self.archived_course.number),
+            ("display_number", self.archived_course.display_number_with_default),
             ("org", self.archived_course.org),
+            ("display_org", self.archived_course.display_org_with_default),
             ("rerun_link", f'/course_rerun/{str(self.archived_course.id)}'),
             ("run", self.archived_course.id.run),
             ("url", f'/course/{str(self.archived_course.id)}'),

cms/djangoapps/contentstore/signals/handlers.py

@@ -189,12 +189,21 @@ def listen_for_library_update(sender, library_key, **kwargs):  # pylint: disable
         update_library_index.delay(str(library_key), datetime.now(UTC).isoformat())
 
 
-@receiver(SignalHandler.item_deleted)
-def handle_item_deleted(**kwargs):
+@receiver(SignalHandler.pre_item_delete)
+def handle_item_deleted(**kwargs) -> None:
     """
-    Receives the item_deleted signal sent by Studio when an XBlock is removed from
-    the course structure and removes any gating milestone data associated with it or
-    its descendants.
+    Receives the pre_item_delete signal sent by Studio when an XBlock is removed
+    from the course structure and removes any gating milestone and upstream link
+    data associated with it or its descendants.
+
+    We use the "pre" signal because once the actual "item_deleted" signal is
+    sent, it's impossible to fetch the descendants of the item.
+
+    NOTE: This partially overlaps with ``delete_upstream_downstream_link_handler``
+    (below), which also removes ComponentLink / ContainerLink rows on delete but
+    only for the single deleted block, not its descendants. This handler is the
+    one responsible for cascading the link deletion to the deleted block's
+    children. Keep the two in sync if you change either.
 
     Arguments:
         kwargs (dict): Contains the content usage key of the item deleted
@@ -211,14 +220,16 @@ def handle_item_deleted(**kwargs):
         try:
             deleted_block = modulestore().get_item(usage_key)
         except ItemNotFoundError:
+            log.warning("Unable to load XBlock %s to handle its pre_item_delete signal", str(usage_key), exc_info=True)
+            # There may be dangling ComponentLink / milestone data.
             return
-        id_list = {deleted_block.location}
+        id_list = {deleted_block.usage_key}
         for block in yield_dynamic_block_descendants(deleted_block, kwargs.get('user_id')):
             # Remove prerequisite milestone data
-            gating_api.remove_prerequisite(block.location)
+            gating_api.remove_prerequisite(block.usage_key)
             # Remove any 'requires' course content milestone relationships
-            gating_api.set_required_content(course_key, block.location, None, None, None)
-            id_list.add(block.location)
+            gating_api.set_required_content(course_key, block.usage_key, None, None, None)
+            id_list.add(block.usage_key)
 
         ComponentLink.objects.filter(downstream_usage_key__in=id_list).delete()
         ContainerLink.objects.filter(downstream_usage_key__in=id_list).delete()
@@ -275,6 +286,12 @@ def update_upstream_downstream_link_handler(**kwargs):
 def delete_upstream_downstream_link_handler(**kwargs):
     """
     Delete upstream->downstream link from database on xblock delete.
+
+    NOTE: This only removes the link for the single deleted block. Cascading the
+    deletion to the block's descendants (e.g. the child components of a deleted
+    container) is handled separately by ``handle_item_deleted`` (above), which
+    listens to the modulestore ``pre_item_delete`` signal. These two handlers
+    partially overlap; keep them in sync if you change either.
     """
     xblock_info = kwargs.get("xblock_info", None)
     if not xblock_info or not isinstance(xblock_info, XBlockData):

cms/djangoapps/contentstore/tasks.py

@@ -56,6 +56,7 @@
 from cms.djangoapps.contentstore.toggles import enable_course_optimizer_check_prev_run_links
 from cms.djangoapps.contentstore.utils import (
     IMPORTABLE_FILE_TYPES,
+    add_instructor,
     contains_course_reference,
     create_course_info_usage_key,
     create_or_update_xblock_upstream_link,
@@ -188,7 +189,14 @@ def rerun_course(source_course_key_string, destination_course_key_string, user_i
         update_unit_discussion_state_from_discussion_blocks(destination_course_key, user_id)
 
         # set initial permissions for the user to access the course.
-        initialize_permissions(destination_course_key, User.objects.get(id=user_id))
+        # NOTE: add_instructor is called here (after clone_course) because when
+        # authz.enable_course_authoring is enabled, it cannot be called pre-task
+        # (CourseOverview doesn't exist yet). This is a temporary workaround until
+        # openedx/openedx-authz#352 is implemented. Once resolved, add_instructor
+        # can move back to the pre-task call site unconditionally.
+        user = User.objects.get(id=user_id)
+        add_instructor(destination_course_key, user, user)
+        initialize_permissions(destination_course_key, user)
 
         # update state: Succeeded
         CourseRerunState.objects.succeeded(course_key=destination_course_key)