feat!: remove learner_downloadable field/flag (#256)

The learner_downloadable flag in ComponentVersionContent was created
with the thought that it would reduce the odds of sensitive assets
being made publicly downloadable by accident (something that has
happened with Course Files & Uploads). But in practice, this flag has
just been a source of unnecessary complexity, and we're more or less
ignoring it in libraries code that calls into openedx-learning.
Instead, we're using the convention that any path starting with static/
is meant to be downloadable–a convention that Learning Core can stay
completely ignorant of.

In addition to removing it from the model, it's removed as an optional
argument from the following public API functions:

- create_component_version_content
- create_next_component_version
- get_redirect_response_for_component_asset

olx_importer/management/commands/load_components.py

@@ -126,7 +126,6 @@ def create_content(self, static_local_path, now, component_version):
             component_version,
             content.id,
             key=key,
-            learner_downloadable=True,
         )
 
     def import_block_type(self, block_type_name, now): # , publish_log_entry):
@@ -177,7 +176,6 @@ def import_block_type(self, block_type_name, now): # , publish_log_entry):
                 component_version,
                 text_content.pk,
                 key="block.xml",
-                learner_downloadable=False
             )
 
             # Cycle through static assets references and add those as well...

openedx_learning/__init__.py

@@ -2,4 +2,4 @@
 Open edX Learning ("Learning Core").
 """
 
-__version__ = "0.16.3"
+__version__ = "0.17.0"

openedx_learning/apps/authoring/components/admin.py

@@ -71,7 +71,6 @@ def get_queryset(self, request):
     fields = [
         "key",
         "format_size",
-        "learner_downloadable",
         "rendered_data",
     ]
     readonly_fields = [

openedx_learning/apps/authoring/components/api.py

@@ -215,7 +215,6 @@ def create_next_component_version(
                     content_id=content_pk,
                     component_version=component_version,
                     key=key,
-                    learner_downloadable=False,
                 )
         # Now copy any old associations that existed, as long as they aren't
         # in conflict with the new stuff or marked for deletion.
@@ -227,7 +226,6 @@ def create_next_component_version(
                     content_id=cvrc.content_id,
                     component_version=component_version,
                     key=cvrc.key,
-                    learner_downloadable=cvrc.learner_downloadable,
                 )
 
         return component_version
@@ -422,7 +420,6 @@ def create_component_version_content(
     content_id: int,
     /,
     key: str,
-    learner_downloadable: bool = False,
 ) -> ComponentVersionContent:
     """
     Add a Content to the given ComponentVersion
@@ -445,15 +442,13 @@ def create_component_version_content(
         component_version_id=component_version_id,
         content_id=content_id,
         key=key,
-        learner_downloadable=learner_downloadable,
     )
     return cvrc
 
 
 class AssetError(StrEnum):
     """Error codes related to fetching ComponentVersion assets."""
     ASSET_PATH_NOT_FOUND_FOR_COMPONENT_VERSION = auto()
-    ASSET_NOT_LEARNER_DOWNLOADABLE = auto()
     ASSET_HAS_NO_DOWNLOAD_FILE = auto()
 
 
@@ -484,7 +479,6 @@ def get_redirect_response_for_component_asset(
     component_version_uuid: UUID,
     asset_path: Path,
     public: bool = False,
-    learner_downloadable_only: bool = True,
 ) -> HttpResponse:
     """
     ``HttpResponse`` for a reverse-proxy to serve a ``ComponentVersion`` asset.
@@ -498,11 +492,6 @@ def get_redirect_response_for_component_asset(
         If ``True``, this will return an ``HttpResponse`` that can be cached in
         a CDN and shared across many clients.
 
-    :param learner_downloadable_only: Only return assets that are meant to be
-        downloadable by Learners, i.e. in the LMS experience. If this is
-        ``True``, then requests for assets that are not meant for student
-        download will return a ``404`` error response.
-
     **Response Codes**
 
     If the asset exists for this ``ComponentVersion``, this function will return
@@ -512,15 +501,10 @@ def get_redirect_response_for_component_asset(
     the ``ComponentVersion`` itself does not exist, the response code will be
     ``404``.
 
-    Other than checking the coarse-grained ``learner_downloadable_only`` flag,
-    *this function does not do auth checking of any sort*–it will never return
+    This function does not do auth checking of any sort. It will never return
     a ``401`` or ``403`` response code. That is by design. Figuring out who is
     making the request and whether they have permission to do so is the
-    responsiblity of whatever is calling this function. The
-    ``learner_downloadable_only`` flag is intended to be a filter for the entire
-    view. When it's True, not even staff can download component-internal assets.
-    This is intended to protect us from accidentally allowing sensitive grading
-    code to get leaked out.
+    responsiblity of whatever is calling this function.
 
     **Metadata Headers**
 
@@ -596,24 +580,6 @@ def _error_header(error: AssetError) -> dict[str, str]:
         )
         return HttpResponseNotFound(headers=info_headers)
 
-    # Check: If we're asking only for Learner Downloadable assets, and the asset
-    # in question is not supposed to be downloadable by learners, then we give a
-    # 404 error. Even staff members are not expected to be able to download
-    # these assets via the LMS endpoint that serves students. Studio would be
-    # expected to have an entirely different view to serve these assets in that
-    # context (along with different timeouts, auth, and cache settings). So in
-    # that sense, the asset doesn't exist for that particular endpoint.
-    if learner_downloadable_only and (not cv_content.learner_downloadable):
-        logger.error(
-           f"ComponentVersion {component_version_uuid} has asset {asset_path}, "
-           "but it is not meant to be downloadable by learners "
-           "(ComponentVersionContent.learner_downloadable=False)."
-        )
-        info_headers.update(
-            _error_header(AssetError.ASSET_NOT_LEARNER_DOWNLOADABLE)
-        )
-        return HttpResponseNotFound(headers=info_headers)
-
     # At this point, we know that there is valid Content that we want to send.
     # This adds Content-level headers, like the hash/etag and content type.
     info_headers.update(contents_api.get_content_info_headers(content))

openedx_learning/apps/authoring/components/migrations/0003_remove_componentversioncontent_learner_downloadable.py

@@ -0,0 +1,17 @@
+# Generated by Django 4.2.16 on 2024-11-06 17:14
+
+from django.db import migrations
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('oel_components', '0002_alter_componentversioncontent_key'),
+    ]
+
+    operations = [
+        migrations.RemoveField(
+            model_name='componentversioncontent',
+            name='learner_downloadable',
+        ),
+    ]

openedx_learning/apps/authoring/components/models.py

@@ -254,43 +254,6 @@ class ComponentVersionContent(models.Model):
     # identifiers that don't map as cleanly to file paths at some point.
     key = key_field(db_column="_key")
 
-    # Long explanation for the ``learner_downloadable`` field:
-    #
-    # Is this Content downloadable during the learning experience? This is
-    # NOT about public vs. private permissions on course assets, as that will be
-    # a policy that can be changed independently of new versions of the content.
-    # For instance, a course team could decide to flip their course assets from
-    # private to public for CDN caching reasons, and that should not require
-    # new ComponentVersions to be created.
-    #
-    # What the ``learner_downloadable`` field refers to is whether this asset is
-    # supposed to *ever* be directly downloadable by browsers during the
-    # learning experience. This will be True for things like images, PDFs, and
-    # video transcript files. This field will be False for things like:
-    #
-    # * Problem Block OLX will contain the answers to the problem. The XBlock
-    #   runtime and ProblemBlock will use this information to generate HTML and
-    #   grade responses, but the the user's browser is never permitted to
-    #   actually download the raw OLX itself.
-    # * Many courses include a python_lib.zip file holding custom Python code
-    #   to be used by codejail to assess student answers. This code will also
-    #   potentially reveal answers, and is never intended to be downloadable by
-    #   the student's browser.
-    # * Some course teams will upload other file formats that their OLX is
-    #   derived from (e.g. specially formatted LaTeX files). These files will
-    #   likewise contain answers and should never be downloadable by the
-    #   student.
-    # * Other custom metadata may be attached as files in the import, such as
-    #   custom identifiers, author information, etc.
-    #
-    # Even if ``learner_downloadble`` is True, the LMS may decide that this
-    # particular student isn't allowed to see this particular piece of content
-    # yet–e.g. because they are not enrolled, or because the exam this Component
-    # is a part of hasn't started yet. That's a matter of LMS permissions and
-    # policy that is not intrinsic to the content itself, and exists at a layer
-    # above this.
-    learner_downloadable = models.BooleanField(default=False)
-
     class Meta:
         constraints = [
             # Uniqueness is only by ComponentVersion and key. If for some reason

openedx_learning/contrib/media_server/views.py

@@ -5,7 +5,7 @@
 """
 from pathlib import Path
 
-from django.core.exceptions import ObjectDoesNotExist, PermissionDenied
+from django.core.exceptions import ObjectDoesNotExist
 from django.http import FileResponse, Http404
 
 from openedx_learning.apps.authoring.components.api import look_up_component_version_content
@@ -34,11 +34,6 @@ def component_asset(
     except ObjectDoesNotExist:
         raise Http404("File not found")  # pylint: disable=raise-missing-from
 
-    if not cvc.learner_downloadable and not (
-        request.user and request.user.is_superuser
-    ):
-        raise PermissionDenied("This file is not publicly downloadable.")
-
     response = FileResponse(cvc.raw_content.file, filename=Path(asset_path).name)
     response["Content-Type"] = cvc.raw_content.mime_type
 

tests/openedx_learning/apps/authoring/components/test_api.py

@@ -388,7 +388,6 @@ def test_add(self):
             new_version.pk,
             new_content.pk,
             key="my/path/to/hello.txt",
-            learner_downloadable=False,
         )
         # re-fetch from the database to check to see if we wrote it correctly
         new_version = components_api.get_component(self.problem.pk) \
@@ -405,7 +404,6 @@ def test_add(self):
             new_version.pk,
             new_content.pk,
             key="//nested/path/hello.txt",
-            learner_downloadable=False,
         )
         new_version = components_api.get_component(self.problem.pk) \
                                     .versions \

tests/openedx_learning/apps/authoring/components/test_assets.py

@@ -75,7 +75,6 @@ def setUpTestData(cls) -> None:
             cls.component_version.pk,
             cls.problem_content.id,
             key="block.xml",
-            learner_downloadable=False,
         )
 
         # Python source file, stored as a file. This is hypothetical, as we
@@ -90,7 +89,6 @@ def setUpTestData(cls) -> None:
             cls.component_version.pk,
             cls.python_source_asset.id,
             key="src/grader.py",
-            learner_downloadable=False,
         )
 
         # An HTML file that is student downloadable
@@ -104,7 +102,6 @@ def setUpTestData(cls) -> None:
             cls.component_version.pk,
             cls.html_asset_content.id,
             key="static/hello.html",
-            learner_downloadable=True,
         )
 
     def test_no_component_version(self):
@@ -145,10 +142,6 @@ def test_404s_with_component_version_info(self):
             # This is testing that asset paths are case sensitive
             "static/HELLO.html": AssetError.ASSET_PATH_NOT_FOUND_FOR_COMPONENT_VERSION,
 
-            # Files that want to guarantee can never be downloaded (they're for
-            # backend usage only).
-            "src/grader.py": AssetError.ASSET_NOT_LEARNER_DOWNLOADABLE,
-
             # Text stored in the database directly instead of file storage.
             "block.xml": AssetError.ASSET_HAS_NO_DOWNLOAD_FILE,
         }