From 3d856a927b9017e1ce4dc20d9a7b6dabbde57e4b Mon Sep 17 00:00:00 2001 From: Cristhian Garcia Date: Thu, 7 Nov 2024 16:07:05 -0500 Subject: [PATCH] feat!: remove learner_downloadable field/flag (#256) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../management/commands/load_components.py | 2 - openedx_learning/__init__.py | 2 +- .../apps/authoring/components/admin.py | 1 - .../apps/authoring/components/api.py | 38 +------------------ ...nentversioncontent_learner_downloadable.py | 17 +++++++++ .../apps/authoring/components/models.py | 37 ------------------ .../contrib/media_server/views.py | 7 +--- .../apps/authoring/components/test_api.py | 2 - .../apps/authoring/components/test_assets.py | 7 ---- 9 files changed, 21 insertions(+), 92 deletions(-) create mode 100644 openedx_learning/apps/authoring/components/migrations/0003_remove_componentversioncontent_learner_downloadable.py diff --git a/olx_importer/management/commands/load_components.py b/olx_importer/management/commands/load_components.py index 0f5ab66c..55cd268c 100644 --- a/olx_importer/management/commands/load_components.py +++ b/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... diff --git a/openedx_learning/__init__.py b/openedx_learning/__init__.py index 1521ec58..23b5c7c2 100644 --- a/openedx_learning/__init__.py +++ b/openedx_learning/__init__.py @@ -2,4 +2,4 @@ Open edX Learning ("Learning Core"). """ -__version__ = "0.16.3" +__version__ = "0.17.0" diff --git a/openedx_learning/apps/authoring/components/admin.py b/openedx_learning/apps/authoring/components/admin.py index edb83921..2fa501dd 100644 --- a/openedx_learning/apps/authoring/components/admin.py +++ b/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 = [ diff --git a/openedx_learning/apps/authoring/components/api.py b/openedx_learning/apps/authoring/components/api.py index ace74fbb..48a45af4 100644 --- a/openedx_learning/apps/authoring/components/api.py +++ b/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,7 +442,6 @@ def create_component_version_content( component_version_id=component_version_id, content_id=content_id, key=key, - learner_downloadable=learner_downloadable, ) return cvrc @@ -453,7 +449,6 @@ def create_component_version_content( 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)) diff --git a/openedx_learning/apps/authoring/components/migrations/0003_remove_componentversioncontent_learner_downloadable.py b/openedx_learning/apps/authoring/components/migrations/0003_remove_componentversioncontent_learner_downloadable.py new file mode 100644 index 00000000..4ae618b7 --- /dev/null +++ b/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', + ), + ] diff --git a/openedx_learning/apps/authoring/components/models.py b/openedx_learning/apps/authoring/components/models.py index df17a999..b0225fc9 100644 --- a/openedx_learning/apps/authoring/components/models.py +++ b/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 diff --git a/openedx_learning/contrib/media_server/views.py b/openedx_learning/contrib/media_server/views.py index 542f041a..d7088879 100644 --- a/openedx_learning/contrib/media_server/views.py +++ b/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 diff --git a/tests/openedx_learning/apps/authoring/components/test_api.py b/tests/openedx_learning/apps/authoring/components/test_api.py index 2a683b05..279a0335 100644 --- a/tests/openedx_learning/apps/authoring/components/test_api.py +++ b/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 \ diff --git a/tests/openedx_learning/apps/authoring/components/test_assets.py b/tests/openedx_learning/apps/authoring/components/test_assets.py index 56bfda41..f9cbf064 100644 --- a/tests/openedx_learning/apps/authoring/components/test_assets.py +++ b/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, }