From 24f9568c4e6247344b2aebcc0bd46d491cb324f3 Mon Sep 17 00:00:00 2001 From: Raul Gallegos Date: Fri, 28 May 2021 12:01:35 -0500 Subject: [PATCH] fix: no library context when not found (#27493) FAL-1848 (OpenCraft) --- .../core/djangoapps/content_libraries/api.py | 2 - .../content_libraries/library_context.py | 4 +- .../content_libraries/tests/base.py | 1 + .../tests/test_content_libraries.py | 50 ++++++++++++++++++- .../core/djangoapps/xblock/rest_api/views.py | 33 +++++++++--- 5 files changed, 79 insertions(+), 11 deletions(-) diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 821f591645..cb5456a717 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -325,7 +325,6 @@ def require_permission_for_library_key(library_key, user, permission): Raises django.core.exceptions.PermissionDenied if the user doesn't have permission. """ - assert isinstance(library_key, LibraryLocatorV2) library_obj = ContentLibrary.objects.get_by_key(library_key) if not user.has_perm(permission, obj=library_obj): raise PermissionDenied @@ -338,7 +337,6 @@ def get_library(library_key): Raises ContentLibraryNotFound if the library doesn't exist. """ - assert isinstance(library_key, LibraryLocatorV2) ref = ContentLibrary.objects.get_by_key(library_key) bundle_metadata = get_bundle(ref.bundle_uuid) lib_bundle = LibraryBundle(library_key, ref.bundle_uuid, draft_name=DRAFT_NAME) diff --git a/openedx/core/djangoapps/content_libraries/library_context.py b/openedx/core/djangoapps/content_libraries/library_context.py index f9316dc972..d25d9a3f3a 100644 --- a/openedx/core/djangoapps/content_libraries/library_context.py +++ b/openedx/core/djangoapps/content_libraries/library_context.py @@ -44,7 +44,7 @@ class LibraryContextImpl(LearningContext): """ try: api.require_permission_for_library_key(usage_key.lib_key, user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) - except PermissionDenied: + except (PermissionDenied, api.ContentLibraryNotFound): return False def_key = self.definition_for_usage(usage_key) if not def_key: @@ -67,7 +67,7 @@ class LibraryContextImpl(LearningContext): api.require_permission_for_library_key( usage_key.lib_key, user, permissions.CAN_LEARN_FROM_THIS_CONTENT_LIBRARY, ) - except PermissionDenied: + except (PermissionDenied, api.ContentLibraryNotFound): return False def_key = self.definition_for_usage(usage_key) if not def_key: diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index 435985571e..465134e8d2 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -40,6 +40,7 @@ URL_LIB_BLOCK_ASSET_FILE = URL_LIB_BLOCK + 'assets/{file_name}' # Get, delete, 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}/' URL_BLOCK_METADATA_URL = '/api/xblock/v2/xblocks/{block_key}/' +URL_BLOCK_XBLOCK_HANDLER = '/api/xblock/v2/xblocks/{block_key}/handler/{user_id}-{secure_token}/{handler_name}/' # Decorator for tests that require blockstore 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 f615863ed1..3f5e2f75dd 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -7,11 +7,20 @@ from unittest.mock import patch import ddt from django.conf import settings from django.contrib.auth.models import Group +from django.test.client import Client from django.test.utils import override_settings from organizations.models import Organization +from rest_framework.test import APITestCase from openedx.core.djangoapps.content_libraries.libraries_index import LibraryBlockIndexer, ContentLibraryIndexer -from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest, elasticsearch_test +from openedx.core.djangoapps.content_libraries.tests.base import ( + ContentLibrariesRestApiTest, + elasticsearch_test, + URL_BLOCK_METADATA_URL, + URL_BLOCK_RENDER_VIEW, + URL_BLOCK_GET_HANDLER_URL, + URL_BLOCK_XBLOCK_HANDLER, +) from openedx.core.djangoapps.content_libraries.constants import VIDEO, COMPLEX, PROBLEM, CC_4_BY, ALL_RIGHTS_RESERVED from common.djangoapps.student.tests.factories import UserFactory @@ -766,3 +775,42 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): assert types[0]['block_type'] == library_type else: assert len(types) > 1 + + +@ddt.ddt +class ContentLibraryXBlockValidationTest(APITestCase): + """Tests only focused on service validation, no Blockstore needed.""" + + @ddt.data( + (URL_BLOCK_METADATA_URL, dict(block_key='totally_invalid_key')), + (URL_BLOCK_RENDER_VIEW, dict(block_key='totally_invalid_key', view_name='random')), + (URL_BLOCK_GET_HANDLER_URL, dict(block_key='totally_invalid_key', handler_name='random')), + ) + @ddt.unpack + def test_invalid_key(self, endpoint, endpoint_parameters): + """Test all xblock related endpoints, when the key is invalid, return 404.""" + response = self.client.get( + endpoint.format(**endpoint_parameters), + ) + self.assertEqual(response.status_code, 404) + self.assertEqual(response.json(), {'detail': "Invalid XBlock key"}) + + def test_xblock_handler_invalid_key(self): + """This endpoint is tested separately from the previous ones as it's not a DRF endpoint.""" + client = Client() + response = client.get(URL_BLOCK_XBLOCK_HANDLER.format(**dict( + block_key='totally_invalid_key', + handler_name='random', + user_id='random', + secure_token='random', + ))) + self.assertEqual(response.status_code, 404) + + def test_not_found_fails_correctly(self): + """Test fails with 404 when xblock key is valid but not found.""" + valid_not_found_key = 'lb:valid:key:video:1' + response = self.client.get(URL_BLOCK_METADATA_URL.format(block_key=valid_not_found_key)) + self.assertEqual(response.status_code, 404) + self.assertEqual(response.json(), { + 'detail': f"XBlock {valid_not_found_key} does not exist, or you don't have permission to view it.", + }) diff --git a/openedx/core/djangoapps/xblock/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py index 92d6fc2af1..7a9059ec52 100644 --- a/openedx/core/djangoapps/xblock/rest_api/views.py +++ b/openedx/core/djangoapps/xblock/rest_api/views.py @@ -7,14 +7,16 @@ Studio APIs cover use cases like adding/deleting/editing blocks. from corsheaders.signals import check_request_enabled from django.contrib.auth import get_user_model +from django.http import Http404 from django.views.decorators.clickjacking import xframe_options_exempt from django.views.decorators.csrf import csrf_exempt from rest_framework import permissions from rest_framework.decorators import api_view, permission_classes # lint-amnesty, pylint: disable=unused-import -from rest_framework.exceptions import PermissionDenied, AuthenticationFailed +from rest_framework.exceptions import PermissionDenied, AuthenticationFailed, NotFound from rest_framework.response import Response from xblock.django.request import DjangoWebobRequest, webob_to_django_response +from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import UsageKey from openedx.core.lib.api.view_utils import view_auth_classes from ..api import ( @@ -22,13 +24,16 @@ from ..api import ( get_handler_url as _get_handler_url, load_block, render_block_view as _render_block_view, - ) from ..utils import validate_secure_token_for_xblock_handler User = get_user_model() +class InvalidNotFound(NotFound): + default_detail = "Invalid XBlock key" + + @api_view(['GET']) @view_auth_classes(is_authenticated=False) @permission_classes((permissions.AllowAny, )) # Permissions are handled at a lower level, by the learning context @@ -39,7 +44,11 @@ def block_metadata(request, usage_key_str): Accepts an "include" query parameter which must be a comma separated list of keys to include. Valid keys are "index_dictionary" and "student_view_data". """ - usage_key = UsageKey.from_string(usage_key_str) + try: + usage_key = UsageKey.from_string(usage_key_str) + except InvalidKeyError as e: + raise InvalidNotFound from e + block = load_block(usage_key, request.user) includes = request.GET.get("include", "").split(",") metadata_dict = get_block_metadata(block, includes=includes) @@ -57,7 +66,11 @@ def render_block_view(request, usage_key_str, view_name): """ Get the HTML, JS, and CSS needed to render the given XBlock. """ - usage_key = UsageKey.from_string(usage_key_str) + try: + usage_key = UsageKey.from_string(usage_key_str) + except InvalidKeyError as e: + raise InvalidNotFound from e + block = load_block(usage_key, request.user) fragment = _render_block_view(block, view_name, request.user) response_data = get_block_metadata(block) @@ -74,7 +87,11 @@ def get_handler_url(request, usage_key_str, handler_name): The URL will expire but is guaranteed to be valid for a minimum of 2 days. """ - usage_key = UsageKey.from_string(usage_key_str) + try: + usage_key = UsageKey.from_string(usage_key_str) + except InvalidKeyError as e: + raise InvalidNotFound from e + handler_url = _get_handler_url(usage_key, handler_name, request.user) return Response({"handler_url": handler_url}) @@ -93,7 +110,10 @@ def xblock_handler(request, user_id, secure_token, usage_key_str, handler_name, auth token included in the URL (see below). As a result it can be exempt from CSRF, session auth, and JWT/OAuth. """ - usage_key = UsageKey.from_string(usage_key_str) + try: + usage_key = UsageKey.from_string(usage_key_str) + except InvalidKeyError as e: + raise Http404 from e # To support sandboxed XBlocks, custom frontends, and other use cases, we # authenticate requests using a secure token in the URL. see @@ -146,4 +166,5 @@ def cors_allow_xblock_handler(sender, request, **kwargs): # lint-amnesty, pylin """ return request.path.startswith('/api/xblock/v2/xblocks/') and '/handler/' in request.path + check_request_enabled.connect(cors_allow_xblock_handler)