diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 294ee677f8..77c94a66dd 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -1,5 +1,8 @@ """ -Python API for content libraries +Python API for content libraries. + +Unless otherwise specified, all APIs in this file deal with the DRAFT version +of the content library. """ from __future__ import absolute_import, division, print_function, unicode_literals from uuid import UUID @@ -31,6 +34,7 @@ from openedx.core.lib.blockstore_api import ( commit_draft, delete_draft, ) +from openedx.core.djangolib import blockstore_cache from openedx.core.djangolib.blockstore_cache import BundleCache from .models import ContentLibrary, ContentLibraryPermission @@ -56,6 +60,10 @@ class LibraryBlockAlreadyExists(KeyError): """ An XBlock with that ID already exists in the library """ +class InvalidNameError(ValueError): + """ The specified name/identifier is not valid """ + + # Models: @attr.s @@ -85,6 +93,21 @@ class LibraryXBlockMetadata(object): has_unpublished_changes = attr.ib(False) +@attr.s +class LibraryXBlockStaticFile(object): + """ + Class that represents a static file in a content library, associated with + a particular XBlock. + """ + # File path e.g. "diagram.png" + # In some rare cases it might contain a folder part, e.g. "en/track1.srt" + path = attr.ib("") + # Publicly accessible URL where the file can be downloaded + url = attr.ib("") + # Size in bytes + size = attr.ib(0) + + @attr.s class LibraryXBlockType(object): """ @@ -260,6 +283,21 @@ def get_library_blocks(library_key): return blocks +def _lookup_usage_key(usage_key): + """ + Given a LibraryUsageLocatorV2 (usage key for an XBlock in a content library) + return the definition key and LibraryBundle + or raise ContentLibraryBlockNotFound + """ + assert isinstance(usage_key, LibraryUsageLocatorV2) + lib_context = get_learning_context_impl(usage_key) + def_key = lib_context.definition_for_usage(usage_key, force_draft=DRAFT_NAME) + if def_key is None: + raise ContentLibraryBlockNotFound(usage_key) + lib_bundle = LibraryBundle(usage_key.lib_key, def_key.bundle_uuid, draft_name=DRAFT_NAME) + return def_key, lib_bundle + + def get_library_block(usage_key): """ Get metadata (LibraryXBlockMetadata) about one specific XBlock in a library @@ -268,12 +306,7 @@ def get_library_block(usage_key): openedx.core.djangoapps.xblock.api.load_block() instead. """ - assert isinstance(usage_key, LibraryUsageLocatorV2) - lib_context = get_learning_context_impl(usage_key) - def_key = lib_context.definition_for_usage(usage_key, force_draft=DRAFT_NAME) - if def_key is None: - raise ContentLibraryBlockNotFound(usage_key) - lib_bundle = LibraryBundle(usage_key.lib_key, def_key.bundle_uuid, draft_name=DRAFT_NAME) + def_key, lib_bundle = _lookup_usage_key(usage_key) return LibraryXBlockMetadata( usage_key=usage_key, def_key=def_key, @@ -371,13 +404,7 @@ def delete_library_block(usage_key, remove_from_parent=True): delete block. This should always be true except when this function calls itself recursively. """ - assert isinstance(usage_key, LibraryUsageLocatorV2) - library_context = get_learning_context_impl(usage_key) - library_ref = ContentLibrary.objects.get_by_key(usage_key.context_key) - def_key = library_context.definition_for_usage(usage_key) - if def_key is None: - raise ContentLibraryBlockNotFound(usage_key) - lib_bundle = LibraryBundle(usage_key.context_key, library_ref.bundle_uuid, draft_name=DRAFT_NAME) + def_key, lib_bundle = _lookup_usage_key(usage_key) # Create a draft: draft_uuid = get_or_create_bundle_draft(def_key.bundle_uuid, DRAFT_NAME).uuid # Does this block have a parent? @@ -395,7 +422,7 @@ def delete_library_block(usage_key, remove_from_parent=True): # we're going to delete this block anyways. delete_library_block(child_usage, remove_from_parent=False) # Delete the definition: - if def_key.bundle_uuid == library_ref.bundle_uuid: + if def_key.bundle_uuid == lib_bundle.bundle_uuid: # This definition is in the library, so delete it: path_prefix = lib_bundle.olx_prefix(def_key) for bundle_file in get_bundle_files(def_key.bundle_uuid, use_draft=DRAFT_NAME): @@ -434,6 +461,74 @@ def create_library_block_child(parent_usage_key, block_type, definition_id): return metadata +def get_library_block_static_asset_files(usage_key): + """ + Given an XBlock in a content library, list all the static asset files + associated with that XBlock. + + Returns a list of LibraryXBlockStaticFile objects. + """ + def_key, lib_bundle = _lookup_usage_key(usage_key) + result = [ + LibraryXBlockStaticFile(path=f.path, url=f.url, size=f.size) + for f in lib_bundle.get_static_files_for_definition(def_key) + ] + result.sort(key=lambda f: f.path) + return result + + +def add_library_block_static_asset_file(usage_key, file_name, file_content): + """ + Upload a static asset file into the library, to be associated with the + specified XBlock. Will silently overwrite an existing file of the same name. + + file_name should be a name like "doc.pdf". It may optionally contain slashes + like 'en/doc.pdf' + file_content should be a binary string. + + Returns a LibraryXBlockStaticFile object. + + Example: + video_block = UsageKey.from_string("lb:VideoTeam:python-intro:video:1") + add_library_block_static_asset_file(video_block, "subtitles-en.srt", subtitles.encode('utf-8')) + """ + assert isinstance(file_content, six.binary_type) + def_key, lib_bundle = _lookup_usage_key(usage_key) + if file_name != file_name.strip().strip('/'): + raise InvalidNameError("file name cannot start/end with / or whitespace.") + if '//' in file_name or '..' in file_name: + raise InvalidNameError("Invalid sequence (// or ..) in filename.") + file_path = lib_bundle.get_static_prefix_for_definition(def_key) + file_name + # Write the new static file into the library bundle's draft + draft = get_or_create_bundle_draft(def_key.bundle_uuid, DRAFT_NAME) + write_draft_file(draft.uuid, file_path, file_content) + # Clear the bundle cache so everyone sees the new file immediately: + lib_bundle.cache.clear() + file_metadata = blockstore_cache.get_bundle_file_metadata_with_cache( + bundle_uuid=def_key.bundle_uuid, path=file_path, draft_name=DRAFT_NAME, + ) + return LibraryXBlockStaticFile(path=file_metadata.path, url=file_metadata.url, size=file_metadata.size) + + +def delete_library_block_static_asset_file(usage_key, file_name): + """ + Delete a static asset file from the library. + + Example: + video_block = UsageKey.from_string("lb:VideoTeam:python-intro:video:1") + delete_library_block_static_asset_file(video_block, "subtitles-en.srt") + """ + def_key, lib_bundle = _lookup_usage_key(usage_key) + if '..' in file_name: + raise InvalidNameError("Invalid .. in file name.") + file_path = lib_bundle.get_static_prefix_for_definition(def_key) + file_name + # Delete the file from the library bundle's draft + draft = get_or_create_bundle_draft(def_key.bundle_uuid, DRAFT_NAME) + write_draft_file(draft.uuid, file_path, contents=None) + # Clear the bundle cache so everyone sees the new file immediately: + lib_bundle.cache.clear() + + def get_allowed_block_types(library_key): # pylint: disable=unused-argument """ Get a list of XBlock types that can be added to the specified content diff --git a/openedx/core/djangoapps/content_libraries/library_bundle.py b/openedx/core/djangoapps/content_libraries/library_bundle.py index e0e3283e5e..5e537cf10f 100644 --- a/openedx/core/djangoapps/content_libraries/library_bundle.py +++ b/openedx/core/djangoapps/content_libraries/library_bundle.py @@ -314,6 +314,34 @@ class LibraryBundle(object): self.cache.set(('has_changes', ), result) return result + def get_static_prefix_for_definition(self, definition_key): + """ + Given a definition key, get the path prefix used for all (public) static + asset files. + + Example: problem/quiz1/static/ + """ + return self.olx_prefix(definition_key) + 'static/' + + def get_static_files_for_definition(self, definition_key): + """ + Return a list of the static asset files related with a particular XBlock + definition. + + If the bundle contains files like: + problem/quiz1/definition.xml + problem/quiz1/static/image1.png + Then this will return + [BundleFile(path="image1.png", size, url, hash_digest)] + """ + path_prefix = self.get_static_prefix_for_definition(definition_key) + path_prefix_len = len(path_prefix) + return [ + blockstore_api.BundleFile(path=f.path[path_prefix_len:], size=f.size, url=f.url, hash_digest=f.hash_digest) + for f in get_bundle_files_cached(self.bundle_uuid, draft_name=self.draft_name) + if f.path.startswith(path_prefix) + ] + @staticmethod def olx_prefix(definition_key): """ diff --git a/openedx/core/djangoapps/content_libraries/serializers.py b/openedx/core/djangoapps/content_libraries/serializers.py index 09190e5548..b438d31cc3 100644 --- a/openedx/core/djangoapps/content_libraries/serializers.py +++ b/openedx/core/djangoapps/content_libraries/serializers.py @@ -7,6 +7,8 @@ from __future__ import absolute_import, division, print_function, unicode_litera from django.core.validators import validate_unicode_slug from rest_framework import serializers +from openedx.core.lib import blockstore_api + class ContentLibraryMetadataSerializer(serializers.Serializer): """ @@ -74,3 +76,35 @@ class LibraryXBlockOlxSerializer(serializers.Serializer): Serializer for representing an XBlock's OLX """ olx = serializers.CharField() + + +class LibraryXBlockStaticFileSerializer(serializers.Serializer): + """ + Serializer representing a static file associated with an XBlock + + Serializes a LibraryXBlockStaticFile (or a BundleFile) + """ + path = serializers.CharField() + # Publicly accessible URL where the file can be downloaded. + # Must be an absolute URL. + url = serializers.URLField() + size = serializers.IntegerField(min_value=0) + + def to_representation(self, instance): + """ + Generate the serialized representation of this static asset file. + """ + result = super(LibraryXBlockStaticFileSerializer, self).to_representation(instance) + # Make sure the URL is one that will work from the user's browser, + # not one that only works from within a docker container: + result['url'] = blockstore_api.force_browser_url(result['url']) + return result + + +class LibraryXBlockStaticFilesSerializer(serializers.Serializer): + """ + Serializer representing a static file associated with an XBlock + + Serializes a LibraryXBlockStaticFile (or a BundleFile) + """ + files = LibraryXBlockStaticFileSerializer(many=True) diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py new file mode 100644 index 0000000000..32362e0295 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -0,0 +1,204 @@ +# -*- coding: utf-8 -*- +""" +Tests for Blockstore-based Content Libraries +""" +from __future__ import absolute_import, division, print_function, unicode_literals +import unittest + +from django.conf import settings +from organizations.models import Organization +from rest_framework.test import APITestCase +import six + +from student.tests.factories import UserFactory +from openedx.core.djangolib.testing.utils import skip_unless_cms +from openedx.core.lib import blockstore_api + +# Define the URLs here - don't use reverse() because we want to detect +# backwards-incompatible changes like changed URLs. +URL_PREFIX = '/api/libraries/v2/' +URL_LIB_CREATE = URL_PREFIX +URL_LIB_DETAIL = URL_PREFIX + '{lib_key}/' # Get data about a library, update or delete library +URL_LIB_BLOCK_TYPES = URL_LIB_DETAIL + 'block_types/' # Get the list of XBlock types that can be added to this library +URL_LIB_COMMIT = URL_LIB_DETAIL + 'commit/' # Commit (POST) or revert (DELETE) all pending changes to this library +URL_LIB_BLOCKS = URL_LIB_DETAIL + 'blocks/' # Get the list of XBlocks in this library, or add a new one +URL_LIB_BLOCK = URL_PREFIX + 'blocks/{block_key}/' # Get data about a block, or delete it +URL_LIB_BLOCK_OLX = URL_LIB_BLOCK + 'olx/' # Get or set the OLX of the specified XBlock +URL_LIB_BLOCK_ASSETS = URL_LIB_BLOCK + 'assets/' # List the static asset files of the specified XBlock +URL_LIB_BLOCK_ASSET_FILE = URL_LIB_BLOCK + 'assets/{file_name}' # Get, delete, or upload a specific static asset file + +URL_BLOCK_RENDER_VIEW = '/api/xblock/v2/xblocks/{block_key}/view/{view_name}/' +URL_BLOCK_GET_HANDLER_URL = '/api/xblock/v2/xblocks/{block_key}/handler_url/{handler_name}/' + + +# Decorator for tests that require blockstore +requires_blockstore = unittest.skipUnless(settings.RUN_BLOCKSTORE_TESTS, "Requires a running Blockstore server") + + +@requires_blockstore +@skip_unless_cms # Content Libraries REST API is only available in Studio +class ContentLibrariesRestApiTest(APITestCase): + """ + Base class for Blockstore-based Content Libraries test that use the REST API + + These tests use the REST API, which in turn relies on the Python API. + Some tests may use the python API directly if necessary to provide + coverage of any code paths not accessible via the REST API. + + In general, these tests should + (1) Use public APIs only - don't directly create data using other methods, + which results in a less realistic test and ties the test suite too + closely to specific implementation details. + (Exception: users can be provisioned using a user factory) + (2) Assert that fields are present in responses, but don't assert that the + entire response has some specific shape. That way, things like adding + new fields to an API response, which are backwards compatible, won't + break any tests, but backwards-incompatible API changes will. + + WARNING: every test should have a unique library slug, because even though + the django/mysql database gets reset for each test case, the lookup between + library slug and bundle UUID does not because it's assumed to be immutable + and cached forever. + """ + + @classmethod + def setUpClass(cls): + super(ContentLibrariesRestApiTest, cls).setUpClass() + cls.user = UserFactory.create(username="Bob", email="bob@example.com", password="edx") + # Create a collection using Blockstore API directly only because there + # is not yet any Studio REST API for doing so: + cls.collection = blockstore_api.create_collection("Content Library Test Collection") + # Create an organization + cls.organization = Organization.objects.create( + name="Content Libraries Tachyon Exploration & Survey Team", + short_name="CL-TEST", + ) + + def setUp(self): + super(ContentLibrariesRestApiTest, self).setUp() + self.client.login(username=self.user.username, password="edx") + + # API helpers + + def _api(self, method, url, data, expect_response): + """ + Call a REST API + """ + response = getattr(self.client, method)(url, data, format="json") + self.assertEqual( + response.status_code, expect_response, + "Unexpected response code {}:\n{}".format(response.status_code, getattr(response, 'data', '(no data)')), + ) + return response.data + + def _create_library(self, slug, title, description="", expect_response=200): + """ Create a library """ + return self._api('post', URL_LIB_CREATE, { + "org": self.organization.short_name, + "slug": slug, + "title": title, + "description": description, + "collection_uuid": str(self.collection.uuid), + }, expect_response) + + def _get_library(self, lib_key, expect_response=200): + """ Get a library """ + return self._api('get', URL_LIB_DETAIL.format(lib_key=lib_key), None, expect_response) + + def _update_library(self, lib_key, **data): + """ Update an existing library """ + return self._api('patch', URL_LIB_DETAIL.format(lib_key=lib_key), data=data, expect_response=200) + + def _delete_library(self, lib_key, expect_response=200): + """ Delete an existing library """ + return self._api('delete', URL_LIB_DETAIL.format(lib_key=lib_key), None, expect_response) + + def _commit_library_changes(self, lib_key): + """ Commit changes to an existing library """ + return self._api('post', URL_LIB_COMMIT.format(lib_key=lib_key), None, expect_response=200) + + def _revert_library_changes(self, lib_key): + """ Revert pending changes to an existing library """ + return self._api('delete', URL_LIB_COMMIT.format(lib_key=lib_key), None, expect_response=200) + + def _get_library_blocks(self, lib_key): + """ Get the list of XBlocks in the library """ + return self._api('get', URL_LIB_BLOCKS.format(lib_key=lib_key), None, expect_response=200) + + def _add_block_to_library(self, lib_key, block_type, slug, parent_block=None, expect_response=200): + """ Add a new XBlock to the library """ + data = {"block_type": block_type, "definition_id": slug} + if parent_block: + data["parent_block"] = parent_block + return self._api('post', URL_LIB_BLOCKS.format(lib_key=lib_key), data, expect_response) + + def _get_library_block(self, block_key, expect_response=200): + """ Get a specific block in the library """ + return self._api('get', URL_LIB_BLOCK.format(block_key=block_key), None, expect_response) + + def _delete_library_block(self, block_key, expect_response=200): + """ Delete a specific block from the library """ + self._api('delete', URL_LIB_BLOCK.format(block_key=block_key), None, expect_response) + + def _get_library_block_olx(self, block_key, expect_response=200): + """ Get the OLX of a specific block in the library """ + result = self._api('get', URL_LIB_BLOCK_OLX.format(block_key=block_key), None, expect_response) + if expect_response == 200: + return result["olx"] + return result + + def _set_library_block_olx(self, block_key, new_olx, expect_response=200): + """ Overwrite the OLX of a specific block in the library """ + return self._api('post', URL_LIB_BLOCK_OLX.format(block_key=block_key), {"olx": new_olx}, expect_response) + + def _get_library_block_assets(self, block_key, expect_response=200): + """ List the static asset files belonging to the specified XBlock """ + url = URL_LIB_BLOCK_ASSETS.format(block_key=block_key) + result = self._api('get', url, None, expect_response) + return result["files"] if expect_response == 200 else result + + def _get_library_block_asset(self, block_key, file_name, expect_response=200): + """ + Get metadata about one static asset file belonging to the specified + XBlock. + """ + url = URL_LIB_BLOCK_ASSET_FILE.format(block_key=block_key, file_name=file_name) + return self._api('get', url, None, expect_response) + + def _set_library_block_asset(self, block_key, file_name, content, expect_response=200): + """ + Set/replace a static asset file belonging to the specified XBlock. + + content should be a binary string. + """ + assert isinstance(content, six.binary_type) + file_handle = six.BytesIO(content) + url = URL_LIB_BLOCK_ASSET_FILE.format(block_key=block_key, file_name=file_name) + response = self.client.put(url, data={"content": file_handle}) + self.assertEqual( + response.status_code, expect_response, + "Unexpected response code {}:\n{}".format(response.status_code, getattr(response, 'data', '(no data)')), + ) + + def _delete_library_block_asset(self, block_key, file_name, expect_response=200): + """ Delete a static asset file. """ + url = URL_LIB_BLOCK_ASSET_FILE.format(block_key=block_key, file_name=file_name) + return self._api('delete', url, None, expect_response) + + def _render_block_view(self, block_key, view_name, expect_response=200): + """ + Render an XBlock's view in the active application's runtime. + Note that this endpoint has different behavior in Studio (draft mode) + vs. the LMS (published version only). + """ + url = URL_BLOCK_RENDER_VIEW.format(block_key=block_key, view_name=view_name) + return self._api('get', url, None, expect_response) + + def _get_block_handler_url(self, block_key, handler_name): + """ + Get the URL to call a specific XBlock's handler. + The URL itself encodes authentication information so can be called + without session authentication or any other kind of authentication. + """ + url = URL_BLOCK_GET_HANDLER_URL.format(block_key=block_key, handler_name=handler_name) + return self._api('get', url, None, expect_response=200)["handler_url"] diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index 34872ec0ae..f523d07292 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -6,34 +6,12 @@ from __future__ import absolute_import, division, print_function, unicode_litera import unittest from uuid import UUID -from django.conf import settings -from organizations.models import Organization -from rest_framework.test import APITestCase - -from student.tests.factories import UserFactory -from openedx.core.djangolib.testing.utils import skip_unless_cms -from openedx.core.lib import blockstore_api - -# Define the URLs here - don't use reverse() because we want to detect -# backwards-incompatible changes like changed URLs. -URL_PREFIX = '/api/libraries/v2/' -URL_LIB_CREATE = URL_PREFIX -URL_LIB_DETAIL = URL_PREFIX + '{lib_key}/' # Get data about a library, update or delete library -URL_LIB_BLOCK_TYPES = URL_LIB_DETAIL + 'block_types/' # Get the list of XBlock types that can be added to this library -URL_LIB_COMMIT = URL_LIB_DETAIL + 'commit/' # Commit (POST) or revert (DELETE) all pending changes to this library -URL_LIB_BLOCKS = URL_LIB_DETAIL + 'blocks/' # Get the list of XBlocks in this library, or add a new one -URL_LIB_BLOCK = URL_PREFIX + 'blocks/{block_key}/' # Get data about a block, or delete it -URL_LIB_BLOCK_OLX = URL_LIB_BLOCK + 'olx/' # Get or set the OLX of the specified XBlock - -URL_BLOCK_RENDER_VIEW = '/api/xblock/v2/xblocks/{block_key}/view/{view_name}/' -URL_BLOCK_GET_HANDLER_URL = '/api/xblock/v2/xblocks/{block_key}/handler_url/{handler_name}/' +from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest -@unittest.skipUnless(settings.RUN_BLOCKSTORE_TESTS, "Requires a running Blockstore server") -@skip_unless_cms # Content Libraries REST API is only available in Studio -class ContentLibrariesTest(APITestCase): +class ContentLibrariesTest(ContentLibrariesRestApiTest): """ - Test for Blockstore-based Content Libraries + General tests for Blockstore-based Content Libraries These tests use the REST API, which in turn relies on the Python API. Some tests may use the python API directly if necessary to provide @@ -55,116 +33,6 @@ class ContentLibrariesTest(APITestCase): and cached forever. """ - @classmethod - def setUpClass(cls): - super(ContentLibrariesTest, cls).setUpClass() - cls.user = UserFactory.create(username="Bob", email="bob@example.com", password="edx") - # Create a collection using Blockstore API directly only because there - # is not yet any Studio REST API for doing so: - cls.collection = blockstore_api.create_collection("Content Library Test Collection") - # Create an organization - cls.organization = Organization.objects.create( - name="Content Libraries Tachyon Exploration & Survey Team", - short_name="CL-TEST", - ) - - def setUp(self): - super(ContentLibrariesTest, self).setUp() - self.client.login(username=self.user.username, password="edx") - - # API helpers - - def _api(self, method, url, data, expect_response): - """ - Call a REST API - """ - response = getattr(self.client, method)(url, data, format="json") - self.assertEqual( - response.status_code, expect_response, - "Unexpected response code {}:\n{}".format(response.status_code, getattr(response, 'data', '(no data)')), - ) - return response.data - - def _create_library(self, slug, title, description="", expect_response=200): - """ Create a library """ - return self._api('post', URL_LIB_CREATE, { - "org": self.organization.short_name, - "slug": slug, - "title": title, - "description": description, - "collection_uuid": str(self.collection.uuid), - }, expect_response) - - def _get_library(self, lib_key, expect_response=200): - """ Get a library """ - return self._api('get', URL_LIB_DETAIL.format(lib_key=lib_key), None, expect_response) - - def _update_library(self, lib_key, **data): - """ Update an existing library """ - return self._api('patch', URL_LIB_DETAIL.format(lib_key=lib_key), data=data, expect_response=200) - - def _delete_library(self, lib_key, expect_response=200): - """ Delete an existing library """ - return self._api('delete', URL_LIB_DETAIL.format(lib_key=lib_key), None, expect_response) - - def _commit_library_changes(self, lib_key): - """ Commit changes to an existing library """ - return self._api('post', URL_LIB_COMMIT.format(lib_key=lib_key), None, expect_response=200) - - def _revert_library_changes(self, lib_key): - """ Revert pending changes to an existing library """ - return self._api('delete', URL_LIB_COMMIT.format(lib_key=lib_key), None, expect_response=200) - - def _get_library_blocks(self, lib_key): - """ Get the list of XBlocks in the library """ - return self._api('get', URL_LIB_BLOCKS.format(lib_key=lib_key), None, expect_response=200) - - def _add_block_to_library(self, lib_key, block_type, slug, parent_block=None, expect_response=200): - """ Add a new XBlock to the library """ - data = {"block_type": block_type, "definition_id": slug} - if parent_block: - data["parent_block"] = parent_block - return self._api('post', URL_LIB_BLOCKS.format(lib_key=lib_key), data, expect_response) - - def _get_library_block(self, block_key, expect_response=200): - """ Get a specific block in the library """ - return self._api('get', URL_LIB_BLOCK.format(block_key=block_key), None, expect_response) - - def _delete_library_block(self, block_key, expect_response=200): - """ Delete a specific block from the library """ - self._api('delete', URL_LIB_BLOCK.format(block_key=block_key), None, expect_response) - - def _get_library_block_olx(self, block_key, expect_response=200): - """ Get the OLX of a specific block in the library """ - result = self._api('get', URL_LIB_BLOCK_OLX.format(block_key=block_key), None, expect_response) - if expect_response == 200: - return result["olx"] - return result - - def _set_library_block_olx(self, block_key, new_olx, expect_response=200): - """ Overwrite the OLX of a specific block in the library """ - return self._api('post', URL_LIB_BLOCK_OLX.format(block_key=block_key), {"olx": new_olx}, expect_response) - - def _render_block_view(self, block_key, view_name, expect_response=200): - """ - Render an XBlock's view in the active application's runtime. - Note that this endpoint has different behavior in Studio (draft mode) - vs. the LMS (published version only). - """ - url = URL_BLOCK_RENDER_VIEW.format(block_key=block_key, view_name=view_name) - return self._api('get', url, None, expect_response) - - def _get_block_handler_url(self, block_key, handler_name): - """ - Get the URL to call a specific XBlock's handler. - The URL itself encodes authentication information so can be called - without session authentication or any other kind of authentication. - """ - url = URL_BLOCK_GET_HANDLER_URL.format(block_key=block_key, handler_name=handler_name) - return self._api('get', url, None, expect_response=200)["handler_url"] - - # General Content Library tests - def test_library_crud(self): """ Test Create, Read, Update, and Delete of a Content Library diff --git a/openedx/core/djangoapps/content_libraries/tests/test_runtime.py b/openedx/core/djangoapps/content_libraries/tests/test_runtime.py index 17dd2cb550..f8048d391d 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_runtime.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_runtime.py @@ -7,7 +7,6 @@ import json import unittest from completion.test_utils import CompletionWaffleTestMixin -from django.conf import settings from django.test import TestCase from organizations.models import Organization from rest_framework.test import APIClient @@ -16,7 +15,8 @@ from xblock import fields from lms.djangoapps.courseware.model_data import get_score from openedx.core.djangoapps.content_libraries import api as library_api -from openedx.core.djangoapps.content_libraries.tests.test_content_libraries import ( +from openedx.core.djangoapps.content_libraries.tests.base import ( + requires_blockstore, URL_BLOCK_RENDER_VIEW, URL_BLOCK_GET_HANDLER_URL, ) @@ -68,7 +68,7 @@ class ContentLibraryContentTestMixin(object): ) -@unittest.skipUnless(settings.RUN_BLOCKSTORE_TESTS, "Requires a running Blockstore server") +@requires_blockstore class ContentLibraryRuntimeTest(ContentLibraryContentTestMixin, TestCase): """ Basic tests of the Blockstore-based XBlock runtime using XBlocks in a @@ -92,7 +92,7 @@ class ContentLibraryRuntimeTest(ContentLibraryContentTestMixin, TestCase): self.assertEqual(problem_block.has_score, True) -@unittest.skipUnless(settings.RUN_BLOCKSTORE_TESTS, "Requires a running Blockstore server") +@requires_blockstore # We can remove the line below to enable this in Studio once we implement a session-backed # field data store which we can use for both studio users and anonymous users @skip_unless_lms @@ -264,7 +264,7 @@ class ContentLibraryXBlockUserStateTest(ContentLibraryContentTestMixin, TestCase self.assertEqual(sm.max_grade, 1) -@unittest.skipUnless(settings.RUN_BLOCKSTORE_TESTS, "Requires a running Blockstore server") +@requires_blockstore @skip_unless_lms # No completion tracking in Studio class ContentLibraryXBlockCompletionTest(ContentLibraryContentTestMixin, CompletionWaffleTestMixin, TestCase): """ diff --git a/openedx/core/djangoapps/content_libraries/tests/test_static_assets.py b/openedx/core/djangoapps/content_libraries/tests/test_static_assets.py new file mode 100644 index 0000000000..66b435d6f3 --- /dev/null +++ b/openedx/core/djangoapps/content_libraries/tests/test_static_assets.py @@ -0,0 +1,113 @@ +# -*- coding: utf-8 -*- +""" +Tests for static asset files in Blockstore-based Content Libraries +""" +from __future__ import absolute_import, division, print_function, unicode_literals + +import requests + +from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest + +# Binary data representing an SVG image file +SVG_DATA = """ + SVG is 🔥 +""".encode('utf-8') + + +class ContentLibrariesStaticAssetsTest(ContentLibrariesRestApiTest): + """ + Tests for static asset files in Blockstore-based Content Libraries + + WARNING: every test should have a unique library slug, because even though + the django/mysql database gets reset for each test case, the lookup between + library slug and bundle UUID does not because it's assumed to be immutable + and cached forever. + """ + + def test_asset_crud(self): + """ + Test create, read, update, and write of a static asset file. + + Also tests that the static asset file (an image in this case) can be + used in an HTML block. + """ + library = self._create_library(slug="asset-lib1", title="Static Assets Test Library") + block = self._add_block_to_library(library["id"], "html", "html1") + block_id = block["id"] + file_name = "image.svg" + + # A new block has no assets: + self.assertEqual(self._get_library_block_assets(block_id), []) + self._get_library_block_asset(block_id, file_name, expect_response=404) + + # Upload an asset file + self._set_library_block_asset(block_id, file_name, SVG_DATA) + + # Get metadata about the uploaded asset file + metadata = self._get_library_block_asset(block_id, file_name) + self.assertEqual(metadata["path"], file_name) + self.assertEqual(metadata["size"], len(SVG_DATA)) + asset_list = self._get_library_block_assets(block_id) + # We don't just assert that 'asset_list == [metadata]' because that may + # break in the future if the "get asset" view returns more detail than + # the "list assets" view. + self.assertEqual(len(asset_list), 1) + self.assertEqual(asset_list[0]["path"], metadata["path"]) + self.assertEqual(asset_list[0]["size"], metadata["size"]) + self.assertEqual(asset_list[0]["url"], metadata["url"]) + + # Download the file and check that it matches what was uploaded. + # We need to download using requests since this is served by Blockstore, + # which the django test client can't interact with. + content_get_result = requests.get(metadata["url"]) + self.assertEqual(content_get_result.content, SVG_DATA) + + # Set some OLX referencing this asset: + self._set_library_block_olx(block_id, """ + + ]]> + """) + # Publish the OLX and the new image file, since published data gets + # served differently by Blockstore and we should test that too. + self._commit_library_changes(library["id"]) + metadata = self._get_library_block_asset(block_id, file_name) + self.assertEqual(metadata["path"], file_name) + self.assertEqual(metadata["size"], len(SVG_DATA)) + # Download the file from the new URL: + content_get_result = requests.get(metadata["url"]) + self.assertEqual(content_get_result.content, SVG_DATA) + + # Check that the URL in the student_view gets rewritten: + fragment = self._render_block_view(block_id, "student_view") + self.assertNotIn("/static/image.svg", fragment["content"]) + self.assertIn(metadata["url"], fragment["content"]) + + def test_asset_filenames(self): + """ + Test various allowed and disallowed filenames + """ + library = self._create_library(slug="asset-lib2", title="Static Assets Test Library") + block = self._add_block_to_library(library["id"], "html", "html1") + block_id = block["id"] + file_size = len(SVG_DATA) + + # Unicode names are allowed + file_name = "🏕.svg" # (camping).svg + self._set_library_block_asset(block_id, file_name, SVG_DATA) + self.assertEqual(self._get_library_block_asset(block_id, file_name)["path"], file_name) + self.assertEqual(self._get_library_block_asset(block_id, file_name)["size"], file_size) + + # Subfolder names are allowed + file_name = "transcripts/en.srt" + self._set_library_block_asset(block_id, file_name, SVG_DATA) + self.assertEqual(self._get_library_block_asset(block_id, file_name)["path"], file_name) + self.assertEqual(self._get_library_block_asset(block_id, file_name)["size"], file_size) + + # '../' is definitely not allowed + file_name = "../definition.xml" + self._set_library_block_asset(block_id, file_name, SVG_DATA, expect_response=400) + + # 'a////////b' is not allowed + file_name = "a////////b" + self._set_library_block_asset(block_id, file_name, SVG_DATA, expect_response=400) diff --git a/openedx/core/djangoapps/content_libraries/urls.py b/openedx/core/djangoapps/content_libraries/urls.py index 7d6f5f5fb5..adea51cd8e 100644 --- a/openedx/core/djangoapps/content_libraries/urls.py +++ b/openedx/core/djangoapps/content_libraries/urls.py @@ -29,9 +29,10 @@ urlpatterns = [ url(r'^$', views.LibraryBlockView.as_view()), # Get the OLX source code of the specified block: url(r'^olx/$', views.LibraryBlockOlxView.as_view()), - # TODO: Publish the draft changes made to this block: - # url(r'^commit/$', views.LibraryBlockCommitView.as_view()), - # View todo: discard draft changes + # CRUD for static asset files associated with a block in the library: + url(r'^assets/$', views.LibraryBlockAssetListView.as_view()), + url(r'^assets/(?P.+)$', views.LibraryBlockAssetView.as_view()), + # Future: publish/discard changes for just this one block # Future: set a block's tags (tags are stored in a Tag bundle and linked in) ])), ])), diff --git a/openedx/core/djangoapps/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index cff56df075..0a21e7148f 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -7,10 +7,11 @@ import logging from opaque_keys.edx.locator import LibraryLocatorV2, LibraryUsageLocatorV2 from organizations.models import Organization +from rest_framework import status from rest_framework.exceptions import NotFound, ValidationError -from rest_framework.views import APIView +from rest_framework.parsers import MultiPartParser from rest_framework.response import Response -#from rest_framework import authentication, permissions +from rest_framework.views import APIView from openedx.core.lib.api.view_utils import view_auth_classes from . import api @@ -21,6 +22,8 @@ from .serializers import ( LibraryXBlockMetadataSerializer, LibraryXBlockTypeSerializer, LibraryXBlockOlxSerializer, + LibraryXBlockStaticFileSerializer, + LibraryXBlockStaticFilesSerializer, ) log = logging.getLogger(__name__) @@ -45,6 +48,9 @@ def convert_exceptions(fn): except api.LibraryBlockAlreadyExists as exc: log.exception(exc.message) raise ValidationError(exc.message) + except api.InvalidNameError as exc: + log.exception(exc.message) + raise ValidationError(exc.message) return wrapped_fn @@ -261,3 +267,69 @@ class LibraryBlockOlxView(APIView): except ValueError as err: raise ValidationError(detail=str(err)) return Response(LibraryXBlockOlxSerializer({"olx": new_olx_str}).data) + + +@view_auth_classes() +class LibraryBlockAssetListView(APIView): + """ + Views to list an existing XBlock's static asset files + """ + @convert_exceptions + def get(self, request, usage_key_str): + """ + List the static asset files belonging to this block. + """ + key = LibraryUsageLocatorV2.from_string(usage_key_str) + files = api.get_library_block_static_asset_files(key) + return Response(LibraryXBlockStaticFilesSerializer({"files": files}).data) + + +@view_auth_classes() +class LibraryBlockAssetView(APIView): + """ + Views to work with an existing XBlock's static asset files + """ + parser_classes = (MultiPartParser, ) + + @convert_exceptions + def get(self, request, usage_key_str, file_path): + """ + Get a static asset file belonging to this block. + """ + key = LibraryUsageLocatorV2.from_string(usage_key_str) + files = api.get_library_block_static_asset_files(key) + for f in files: + if f.path == file_path: + return Response(LibraryXBlockStaticFileSerializer(f).data) + raise NotFound + + @convert_exceptions + def put(self, request, usage_key_str, file_path): + """ + Replace a static asset file belonging to this block. + """ + usage_key = LibraryUsageLocatorV2.from_string(usage_key_str) + file_wrapper = request.data['content'] + if file_wrapper.size > 20 * 1024 * 1024: # > 20 MiB + # In the future, we need a way to use file_wrapper.chunks() to read + # the file in chunks and stream that to Blockstore, but Blockstore + # currently lacks an API for streaming file uploads. + raise ValidationError("File too big") + file_content = file_wrapper.read() + try: + result = api.add_library_block_static_asset_file(usage_key, file_path, file_content) + except ValueError: + raise ValidationError("Invalid file path") + return Response(LibraryXBlockStaticFileSerializer(result).data) + + @convert_exceptions + def delete(self, request, usage_key_str, file_path): # pylint: disable=unused-argument + """ + Delete a static asset file belonging to this block. + """ + usage_key = LibraryUsageLocatorV2.from_string(usage_key_str) + try: + api.delete_library_block_static_asset_file(usage_key, file_path) + except ValueError: + raise ValidationError("Invalid file path") + return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py index a7a6a0224d..4e75bd489b 100644 --- a/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/blockstore_runtime.py @@ -4,6 +4,7 @@ XBlock field data directly from Blockstore. """ from __future__ import absolute_import, division, print_function, unicode_literals import logging +import os.path from lxml import etree from opaque_keys.edx.locator import BundleDefinitionLocator @@ -14,7 +15,11 @@ from openedx.core.djangoapps.xblock.learning_context.manager import get_learning from openedx.core.djangoapps.xblock.runtime.runtime import XBlockRuntime from openedx.core.djangoapps.xblock.runtime.olx_parsing import parse_xblock_include from openedx.core.djangoapps.xblock.runtime.serializer import serialize_xblock -from openedx.core.djangolib.blockstore_cache import BundleCache, get_bundle_file_data_with_cache +from openedx.core.djangolib.blockstore_cache import ( + BundleCache, + get_bundle_file_data_with_cache, + get_bundle_file_metadata_with_cache, +) from openedx.core.lib import blockstore_api log = logging.getLogger(__name__) @@ -148,13 +153,45 @@ class BlockstoreXBlockRuntime(XBlockRuntime): olx_path = definition_key.olx_path blockstore_api.write_draft_file(draft_uuid, olx_path, olx_str) # And the other files, if any: + olx_static_path = os.path.dirname(olx_path) + '/static/' for fh in static_files: - raise NotImplementedError( - "Need to write static file {} to blockstore but that's not yet implemented yet".format(fh.name) - ) + new_path = olx_static_path + fh.name + blockstore_api.write_draft_file(draft_uuid, new_path, fh.data) # Now invalidate the blockstore data cache for the bundle: BundleCache(definition_key.bundle_uuid, draft_name=definition_key.draft_name).clear() + def _lookup_asset_url(self, block, asset_path): + """ + Return an absolute URL for the specified static asset file that may + belong to this XBlock. + + e.g. if the XBlock settings have a field value like "/static/foo.png" + then this method will be called with asset_path="foo.png" and should + return a URL like https://cdn.none/xblock/f843u89789/static/foo.png + + If the asset file is not recognized, return None + """ + if '..' in asset_path: + return None # Illegal path + definition_key = block.scope_ids.def_id + # Compute the full path to the static file in the bundle, + # e.g. "problem/prob1/static/illustration.svg" + expanded_path = os.path.dirname(definition_key.olx_path) + '/static/' + asset_path + try: + metadata = get_bundle_file_metadata_with_cache( + bundle_uuid=definition_key.bundle_uuid, + path=expanded_path, + bundle_version=definition_key.bundle_version, + draft_name=definition_key.draft_name, + ) + except blockstore_api.BundleFileNotFound: + log.warning("XBlock static file not found: %s for %s", asset_path, block.scope_ids.usage_id) + return None + # Make sure the URL is one that will work from the user's browser, + # not one that only works from within a docker container: + url = blockstore_api.force_browser_url(metadata.url) + return url + def xml_for_definition(definition_key): """ diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index b0df80894f..e5f60f4a5b 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -25,7 +25,8 @@ from lms.djangoapps.grades.api import signals as grades_signals from openedx.core.djangoapps.xblock.apps import get_xblock_app_config from openedx.core.djangoapps.xblock.runtime.blockstore_field_data import BlockstoreFieldData from openedx.core.djangoapps.xblock.runtime.mixin import LmsBlockMixin -from openedx.core.lib.xblock_utils import xblock_local_resource_url +from openedx.core.lib.xblock_utils import wrap_fragment, xblock_local_resource_url +from static_replace import process_static_urls from xmodule.errortracker import make_error_tracker from .id_managers import OpaqueKeyReader from .shims import RuntimeShim, XBlockShim @@ -289,8 +290,60 @@ class XBlockRuntime(RuntimeShim, Runtime): log.debug("-> Relative resource URL: %s", resource['data']) resource['data'] = get_xblock_app_config().get_site_root_url() + resource['data'] fragment = Fragment.from_dict(frag_data) + + # Apply any required transforms to the fragment. + # We could move to doing this in wrap_xblock() and/or use an array of + # wrapper methods like the ConfigurableFragmentWrapper mixin does. + fragment = wrap_fragment(fragment, self.transform_static_paths_to_urls(block, fragment.content)) + return fragment + def transform_static_paths_to_urls(self, block, html_str): + """ + Given an HTML string, replace any static file paths like + /static/foo.png + (which are really pointing to block-specific assets stored in blockstore) + with working absolute URLs like + https://s3.example.com/blockstore/bundle17/this-block/assets/324.png + See common/djangoapps/static_replace/__init__.py + + This is generally done automatically for the HTML rendered by XBlocks, + but if an XBlock wants to have correct URLs in data returned by its + handlers, the XBlock must call this API directly. + + Note that the paths are only replaced if they are in "quotes" such as if + they are an HTML attribute or JSON data value. Thus, to transform only a + single path string on its own, you must pass html_str=f'"{path}"' + """ + + def replace_static_url(original, prefix, quote, rest): # pylint: disable=unused-argument + """ + Replace a single matched url. + """ + original_url = prefix + rest + # Don't mess with things that end in '?raw' + if rest.endswith('?raw'): + new_url = original_url + else: + new_url = self._lookup_asset_url(block, rest) or original_url + return "".join([quote, new_url, quote]) + + return process_static_urls(html_str, replace_static_url) + + def _lookup_asset_url(self, block, asset_path): # pylint: disable=unused-argument + """ + Return an absolute URL for the specified static asset file that may + belong to this XBlock. + + e.g. if the XBlock settings have a field value like "/static/foo.png" + then this method will be called with asset_path="foo.png" and should + return a URL like https://cdn.none/xblock/f843u89789/static/foo.png + + If the asset file is not recognized, return None + """ + # Subclasses should override this + return None + class XBlockRuntimeSystem(object): """ diff --git a/openedx/core/djangoapps/xblock/runtime/shims.py b/openedx/core/djangoapps/xblock/runtime/shims.py index 6e9189ebd5..d01c487868 100644 --- a/openedx/core/djangoapps/xblock/runtime/shims.py +++ b/openedx/core/djangoapps/xblock/runtime/shims.py @@ -11,9 +11,9 @@ from django.core.cache import cache from django.template import TemplateDoesNotExist from django.utils.functional import cached_property from fs.memoryfs import MemoryFS +import six from edxmako.shortcuts import render_to_string -import six class RuntimeShim(object): @@ -184,17 +184,24 @@ class RuntimeShim(object): def replace_urls(self, html_str): """ - Given an HTML string, replace any static file URLs like + Deprecated precursor to transform_static_paths_to_urls + + Given an HTML string, replace any static file paths like /static/foo.png - with working URLs like - https://s3.example.com/course/this/assets/foo.png + (which are really pointing to block-specific assets stored in blockstore) + with working absolute URLs like + https://s3.example.com/blockstore/bundle17/this-block/assets/324.png See common/djangoapps/static_replace/__init__.py + + This is generally done automatically for the HTML rendered by XBlocks, + but if an XBlock wants to have correct URLs in data returned by its + handlers, the XBlock must call this API directly. + + Note that the paths are only replaced if they are in "quotes" such as if + they are an HTML attribute or JSON data value. Thus, to transform only a + single path string on its own, you must pass html_str=f'"{path}"' """ - # TODO: implement or deprecate. - # Can we replace this with a filesystem service that has a .get_url - # method on all files? See comments in the 'resources_fs' property. - # See also the version in openedx/core/lib/xblock_utils/__init__.py - return html_str # For now just return without changes. + return self.transform_static_paths_to_urls(self._active_block, html_str) def replace_course_urls(self, html_str): """ diff --git a/openedx/core/djangoapps/xblock/tests/README.rst b/openedx/core/djangoapps/xblock/tests/README.rst new file mode 100644 index 0000000000..14f350c678 --- /dev/null +++ b/openedx/core/djangoapps/xblock/tests/README.rst @@ -0,0 +1,8 @@ +XBlock App Suite Tests +====================== + +Where are they? + +As much of the runtime and XBlock API code depends on a learning context, the +XBlock and XBlock Runtime APIs (both python and REST) in this django app are +tested via tests found in `content_libraries/tests <../../content_libraries/tests>`_. diff --git a/openedx/core/lib/blockstore_api/__init__.py b/openedx/core/lib/blockstore_api/__init__.py index 41aa070c23..288aeada6c 100644 --- a/openedx/core/lib/blockstore_api/__init__.py +++ b/openedx/core/lib/blockstore_api/__init__.py @@ -42,6 +42,8 @@ from .methods import ( # Links: get_bundle_links, get_bundle_version_links, + # Misc: + force_browser_url, ) from .exceptions import ( BlockstoreException, diff --git a/openedx/core/lib/blockstore_api/methods.py b/openedx/core/lib/blockstore_api/methods.py index 89f3619c1b..a2ca8297ff 100644 --- a/openedx/core/lib/blockstore_api/methods.py +++ b/openedx/core/lib/blockstore_api/methods.py @@ -390,3 +390,20 @@ def encode_str_for_draft(input_str): else: binary = input_str return base64.b64encode(binary) + + +def force_browser_url(blockstore_file_url): + """ + Ensure that the given URL Blockstore is a URL accessible from the end user's + browser. + """ + # Hack: on some devstacks, we must necessarily use different URLs for + # accessing Blockstore file data from within and outside of docker + # containers, but Blockstore has no way of knowing which case any particular + # request is for. So it always returns a URL suitable for use from within + # the container. Only this edxapp can transform the URL at the last second, + # knowing that in this case it's going to the user's browser and not being + # read by edxapp. + # In production, the same S3 URLs get used for internal and external access + # so this hack is not necessary. + return blockstore_file_url.replace('http://edx.devstack.blockstore:', 'http://localhost:')