diff --git a/cms/templates/content_libraries/xblock_iframe.html b/common/templates/xblock_v2/xblock_iframe.html similarity index 99% rename from cms/templates/content_libraries/xblock_iframe.html rename to common/templates/xblock_v2/xblock_iframe.html index b6e455f785..8b733373bd 100644 --- a/cms/templates/content_libraries/xblock_iframe.html +++ b/common/templates/xblock_v2/xblock_iframe.html @@ -156,7 +156,7 @@ - + {{ fragment.body_html | safe }} diff --git a/lms/templates/xblock_v2/xblock_iframe.html b/lms/templates/xblock_v2/xblock_iframe.html deleted file mode 120000 index 7264c25346..0000000000 --- a/lms/templates/xblock_v2/xblock_iframe.html +++ /dev/null @@ -1 +0,0 @@ -../../../cms/templates/content_libraries/xblock_iframe.html \ No newline at end of file diff --git a/openedx/core/djangoapps/content_libraries/library_context.py b/openedx/core/djangoapps/content_libraries/library_context.py index 6ff426e735..4bda10eb12 100644 --- a/openedx/core/djangoapps/content_libraries/library_context.py +++ b/openedx/core/djangoapps/content_libraries/library_context.py @@ -1,19 +1,21 @@ """ Definition of "Library" as a learning context. """ - import logging from django.core.exceptions import PermissionDenied +from rest_framework.exceptions import NotFound from openedx_events.content_authoring.data import LibraryBlockData from openedx_events.content_authoring.signals import LIBRARY_BLOCK_UPDATED +from opaque_keys.edx.keys import UsageKeyV2 +from opaque_keys.edx.locator import LibraryUsageLocatorV2, LibraryLocatorV2 +from openedx_learning.api import authoring as authoring_api from openedx.core.djangoapps.content_libraries import api, permissions from openedx.core.djangoapps.content_libraries.models import ContentLibrary from openedx.core.djangoapps.xblock.api import LearningContext - -from openedx_learning.api import authoring as authoring_api +from openedx.core.types import User as UserType log = logging.getLogger(__name__) @@ -30,47 +32,51 @@ class LibraryContextImpl(LearningContext): super().__init__(**kwargs) self.use_draft = kwargs.get('use_draft', None) - def can_edit_block(self, user, usage_key): + def can_edit_block(self, user: UserType, usage_key: UsageKeyV2) -> bool: """ - Does the specified usage key exist in its context, and if so, does the - specified user have permission to edit it (make changes to the authored - data store)? + Assuming a block with the specified ID (usage_key) exists, does the + specified user have permission to edit it (make changes to the + fields / authored data store)? - user: a Django User object (may be an AnonymousUser) - - usage_key: the UsageKeyV2 subclass used for this learning context - - Must return a boolean. + May raise ContentLibraryNotFound if the library does not exist. """ - try: - api.require_permission_for_library_key(usage_key.lib_key, user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) - except (PermissionDenied, api.ContentLibraryNotFound): - return False + assert isinstance(usage_key, LibraryUsageLocatorV2) + return self._check_perm(user, usage_key.lib_key, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY) - return self.block_exists(usage_key) + def can_view_block_for_editing(self, user: UserType, usage_key: UsageKeyV2) -> bool: + """ + Assuming a block with the specified ID (usage_key) exists, does the + specified user have permission to view its fields and OLX details (but + not necessarily to make changes to it)? - def can_view_block(self, user, usage_key): + May raise ContentLibraryNotFound if the library does not exist. + """ + assert isinstance(usage_key, LibraryUsageLocatorV2) + return self._check_perm(user, usage_key.lib_key, permissions.CAN_VIEW_THIS_CONTENT_LIBRARY) + + def can_view_block(self, user: UserType, usage_key: UsageKeyV2) -> bool: """ Does the specified usage key exist in its context, and if so, does the specified user have permission to view it and interact with it (call handlers, save user state, etc.)? - user: a Django User object (may be an AnonymousUser) - - usage_key: the UsageKeyV2 subclass used for this learning context - - Must return a boolean. + May raise ContentLibraryNotFound if the library does not exist. """ + assert isinstance(usage_key, LibraryUsageLocatorV2) + return self._check_perm(user, usage_key.lib_key, permissions.CAN_LEARN_FROM_THIS_CONTENT_LIBRARY) + + def _check_perm(self, user: UserType, lib_key: LibraryLocatorV2, perm) -> bool: + """ Helper method to check a permission for the various can_ methods""" try: - api.require_permission_for_library_key( - usage_key.lib_key, user, permissions.CAN_LEARN_FROM_THIS_CONTENT_LIBRARY, - ) - except (PermissionDenied, api.ContentLibraryNotFound): + api.require_permission_for_library_key(lib_key, user, perm) + return True + except PermissionDenied: return False + except api.ContentLibraryNotFound as exc: + # A 404 is probably what you want in this case, not a 500 error, so do that by default. + raise NotFound(f"Content Library '{lib_key}' does not exist") from exc - return self.block_exists(usage_key) - - def block_exists(self, usage_key): + def block_exists(self, usage_key: LibraryUsageLocatorV2): """ Does the block for this usage_key exist in this Library? @@ -82,7 +88,7 @@ class LibraryContextImpl(LearningContext): version of it. """ try: - content_lib = ContentLibrary.objects.get_by_key(usage_key.context_key) + content_lib = ContentLibrary.objects.get_by_key(usage_key.context_key) # type: ignore[attr-defined] except ContentLibrary.DoesNotExist: return False @@ -97,12 +103,11 @@ class LibraryContextImpl(LearningContext): local_key=usage_key.block_id, ) - def send_block_updated_event(self, usage_key): + def send_block_updated_event(self, usage_key: UsageKeyV2): """ Send a "block updated" event for the library block with the given usage_key. - - usage_key: the UsageKeyV2 subclass used for this learning context """ + assert isinstance(usage_key, LibraryUsageLocatorV2) LIBRARY_BLOCK_UPDATED.send_event( library_block=LibraryBlockData( library_key=usage_key.lib_key, diff --git a/openedx/core/djangoapps/content_libraries/permissions.py b/openedx/core/djangoapps/content_libraries/permissions.py index c7da012c9f..17671b5659 100644 --- a/openedx/core/djangoapps/content_libraries/permissions.py +++ b/openedx/core/djangoapps/content_libraries/permissions.py @@ -2,7 +2,8 @@ Permissions for Content Libraries (v2, Learning-Core-based) """ from bridgekeeper import perms, rules -from bridgekeeper.rules import Attribute, ManyRelation, Relation, in_current_groups +from bridgekeeper.rules import Attribute, ManyRelation, Relation, blanket_rule, in_current_groups +from django.conf import settings from openedx.core.djangoapps.content_libraries.models import ContentLibraryPermission @@ -41,6 +42,12 @@ has_explicit_admin_permission_for_library = ( ) +# Are we in Studio? (Is there a better or more contextual way to define this, e.g. get from learning context?) +@blanket_rule +def is_studio_request(_): + return settings.SERVICE_VARIANT == "cms" + + ########################### Permissions ########################### # Is the user allowed to view XBlocks from the specified content library @@ -51,10 +58,12 @@ CAN_LEARN_FROM_THIS_CONTENT_LIBRARY = 'content_libraries.learn_from_library' perms[CAN_LEARN_FROM_THIS_CONTENT_LIBRARY] = ( # Global staff can learn from any library: is_global_staff | - # Regular users can learn if the library allows public learning: + # Regular and even anonymous users can learn if the library allows public learning: Attribute('allow_public_learning', True) | # Users/groups who are explicitly granted permission can learn from the library: - (is_user_active & has_explicit_read_permission_for_library) + (is_user_active & has_explicit_read_permission_for_library) | + # Or, in Studio (but not the LMS) any users can access libraries with "public read" permissions: + (is_studio_request & is_user_active & Attribute('allow_public_read', True)) ) # Is the user allowed to create content libraries? diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index 987133255f..0ed47f995f 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -308,3 +308,12 @@ class ContentLibrariesRestApiTest(APITransactionTestCase): """ 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"] + + def _get_library_block_fields(self, block_key, expect_response=200): + """ Get the fields of a specific block in the library. This API is only used by the MFE editors. """ + result = self._api('get', URL_BLOCK_FIELDS_URL.format(block_key=block_key), None, expect_response) + return result + + def _set_library_block_fields(self, block_key, new_fields, expect_response=200): + """ Set the fields of a specific block in the library. This API is only used by the MFE editors. """ + return self._api('post', URL_BLOCK_FIELDS_URL.format(block_key=block_key), new_fields, expect_response) 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 d995a2c796..56e258f8a1 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -502,6 +502,30 @@ class ContentLibrariesTestCase(ContentLibrariesRestApiTest, OpenEdxEventsTestMix # Add a 'problem' XBlock to the library: self._add_block_to_library(lib_id, block_type, 'test-block', expect_response=expect_response) + def test_library_not_found(self): + """Test that requests fail with 404 when the library does not exist""" + 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': "Content Library 'lib:valid:key' does not exist", + }) + + def test_block_not_found(self): + """Test that requests fail with 404 when the library exists but the XBlock does not""" + lib = self._create_library( + slug="test_lib_block_event_delete", + title="Event Test Library", + description="Testing event in library" + ) + library_key = LibraryLocatorV2.from_string(lib['id']) + non_existent_block_key = LibraryUsageLocatorV2(lib_key=library_key, block_type='video', usage_id='123') + response = self.client.get(URL_BLOCK_METADATA_URL.format(block_key=non_existent_block_key)) + self.assertEqual(response.status_code, 404) + self.assertEqual(response.json(), { + 'detail': f"The component '{non_existent_block_key}' does not exist.", + }) + # Test that permissions are enforced for content libraries def test_library_permissions(self): # pylint: disable=too-many-statements @@ -635,21 +659,27 @@ class ContentLibrariesTestCase(ContentLibrariesRestApiTest, OpenEdxEventsTestMix # A random user cannot read OLX nor assets (this library has allow_public_read False): with self.as_user(random_user): self._get_library_block_olx(block3_key, expect_response=403) + self._get_library_block_fields(block3_key, expect_response=403) self._get_library_block_assets(block3_key, expect_response=403) self._get_library_block_asset(block3_key, file_name="whatever.png", expect_response=403) + # Nor can they preview the block: + self._render_block_view(block3_key, view_name="student_view", expect_response=403) # But if we grant allow_public_read, then they can: with self.as_user(admin): self._update_library(lib_id, allow_public_read=True) # self._set_library_block_asset(block3_key, "whatever.png", b"data") with self.as_user(random_user): self._get_library_block_olx(block3_key) + self._render_block_view(block3_key, view_name="student_view") + f = self._get_library_block_fields(block3_key) # self._get_library_block_assets(block3_key) # self._get_library_block_asset(block3_key, file_name="whatever.png") - # Users without authoring permission cannot edit nor delete XBlocks (this library has allow_public_read False): + # Users without authoring permission cannot edit nor delete XBlocks: for user in [reader, random_user]: with self.as_user(user): self._set_library_block_olx(block3_key, "", expect_response=403) + self._set_library_block_fields(block3_key, {"data": "", "metadata": {}}, expect_response=403) # self._set_library_block_asset(block3_key, "test.txt", b"data", expect_response=403) self._delete_library_block(block3_key, expect_response=403) self._commit_library_changes(lib_id, expect_response=403) @@ -659,6 +689,7 @@ class ContentLibrariesTestCase(ContentLibrariesRestApiTest, OpenEdxEventsTestMix with self.as_user(author_group_member): olx = self._get_library_block_olx(block3_key) self._set_library_block_olx(block3_key, olx) + self._set_library_block_fields(block3_key, {"data": olx, "metadata": {}}) # self._get_library_block_assets(block3_key) # self._set_library_block_asset(block3_key, "test.txt", b"data") # self._get_library_block_asset(block3_key, file_name="test.txt") @@ -1087,12 +1118,3 @@ class ContentLibraryXBlockValidationTest(APITestCase): 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/content_libraries/views.py b/openedx/core/djangoapps/content_libraries/views.py index 835a5de1f1..3712af6e59 100644 --- a/openedx/core/djangoapps/content_libraries/views.py +++ b/openedx/core/djangoapps/content_libraries/views.py @@ -919,7 +919,7 @@ class LtiToolLaunchView(TemplateResponseMixin, LtiToolView): LTI platform. Other features and resouces are ignored. """ - template_name = 'content_libraries/xblock_iframe.html' + template_name = 'xblock_v2/xblock_iframe.html' @property def launch_data(self): diff --git a/openedx/core/djangoapps/xblock/api.py b/openedx/core/djangoapps/xblock/api.py index 80a4bd57ea..43ec3909bc 100644 --- a/openedx/core/djangoapps/xblock/api.py +++ b/openedx/core/djangoapps/xblock/api.py @@ -8,11 +8,12 @@ Note that these views are only for interacting with existing blocks. Other Studio APIs cover use cases like adding/deleting/editing blocks. """ # pylint: disable=unused-import - +from enum import Enum from datetime import datetime import logging import threading +from django.core.exceptions import PermissionDenied from django.urls import reverse from django.utils.translation import gettext as _ from openedx_learning.api import authoring as authoring_api @@ -21,7 +22,7 @@ from opaque_keys.edx.keys import UsageKeyV2 from opaque_keys.edx.locator import BundleDefinitionLocator, LibraryUsageLocatorV2 from rest_framework.exceptions import NotFound from xblock.core import XBlock -from xblock.exceptions import NoSuchViewError +from xblock.exceptions import NoSuchUsage, NoSuchViewError from xblock.plugin import PluginMissingError from openedx.core.djangoapps.xblock.apps import get_xblock_app_config @@ -43,6 +44,16 @@ from openedx.core.djangoapps.xblock.learning_context import LearningContext log = logging.getLogger(__name__) +class CheckPerm(Enum): + """ Options for the default permission check done by load_block() """ + # can view the published block and call handlers etc. but not necessarily view its OLX source nor field data + CAN_LEARN = 1 + # read-only studio view: can see the block (draft or published), see its OLX, see its field data, etc. + CAN_READ_AS_AUTHOR = 2 + # can view everything and make changes to the block + CAN_EDIT = 3 + + def get_runtime_system(): """ Return a new XBlockRuntimeSystem. @@ -74,15 +85,15 @@ def get_runtime_system(): return runtime -def load_block(usage_key, user): +def load_block(usage_key, user, *, check_permission: CheckPerm | None = CheckPerm.CAN_LEARN): """ Load the specified XBlock for the given user. Returns an instantiated XBlock. Exceptions: - NotFound - if the XBlock doesn't exist or if the user doesn't have the - necessary permissions + NotFound - if the XBlock doesn't exist + PermissionDenied - if the user doesn't have the necessary permissions Args: usage_key(OpaqueKey): block identifier @@ -94,10 +105,17 @@ def load_block(usage_key, user): # Now, check if the block exists in this context and if the user has # permission to render this XBlock view: - if user is not None and not context_impl.can_view_block(user, usage_key): - # We do not know if the block was not found or if the user doesn't have - # permission, but we want to return the same result in either case: - raise NotFound(f"XBlock {usage_key} does not exist, or you don't have permission to view it.") + if check_permission and user is not None: + if check_permission == CheckPerm.CAN_EDIT: + has_perm = context_impl.can_edit_block(user, usage_key) + elif check_permission == CheckPerm.CAN_READ_AS_AUTHOR: + has_perm = context_impl.can_view_block_for_editing(user, usage_key) + elif check_permission == CheckPerm.CAN_LEARN: + has_perm = context_impl.can_view_block(user, usage_key) + else: + has_perm = False + if not has_perm: + raise PermissionDenied(f"You don't have permission to access the component '{usage_key}'.") # TODO: load field overrides from the context # e.g. a course might specify that all 'problem' XBlocks have 'max_attempts' @@ -105,7 +123,11 @@ def load_block(usage_key, user): # field_overrides = context_impl.get_field_overrides(usage_key) runtime = get_runtime_system().get_runtime(user=user) - return runtime.get_block(usage_key) + try: + return runtime.get_block(usage_key) + except NoSuchUsage as exc: + # Convert NoSuchUsage to NotFound so we do the right thing (404 not 500) by default. + raise NotFound(f"The component '{usage_key}' does not exist.") from exc def get_block_metadata(block, includes=()): diff --git a/openedx/core/djangoapps/xblock/learning_context/learning_context.py b/openedx/core/djangoapps/xblock/learning_context/learning_context.py index 2dc5155dc4..b535e84ca7 100644 --- a/openedx/core/djangoapps/xblock/learning_context/learning_context.py +++ b/openedx/core/djangoapps/xblock/learning_context/learning_context.py @@ -2,6 +2,8 @@ A "Learning Context" is a course, a library, a program, or some other collection of content where learning happens. """ +from openedx.core.types import User as UserType +from opaque_keys.edx.keys import UsageKeyV2 class LearningContext: @@ -23,11 +25,11 @@ class LearningContext: parameters without changing the API. """ - def can_edit_block(self, user, usage_key): # pylint: disable=unused-argument + def can_edit_block(self, user: UserType, usage_key: UsageKeyV2) -> bool: # pylint: disable=unused-argument """ - Does the specified usage key exist in its context, and if so, does the - specified user have permission to edit it (make changes to the authored - data store)? + Assuming a block with the specified ID (usage_key) exists, does the + specified user have permission to edit it (make changes to the + fields / authored data store)? user: a Django User object (may be an AnonymousUser) @@ -37,11 +39,20 @@ class LearningContext: """ return False - def can_view_block(self, user, usage_key): # pylint: disable=unused-argument + def can_view_block_for_editing(self, user: UserType, usage_key: UsageKeyV2) -> bool: """ - Does the specified usage key exist in its context, and if so, does the + Assuming a block with the specified ID (usage_key) exists, does the + specified user have permission to view its fields and OLX details (but + not necessarily to make changes to it)? + """ + return self.can_edit_block(user, usage_key) + + def can_view_block(self, user: UserType, usage_key: UsageKeyV2) -> bool: # pylint: disable=unused-argument + """ + Assuming a block with the specified ID (usage_key) exists, does the specified user have permission to view it and interact with it (call - handlers, save user state, etc.)? + handlers, save user state, etc.)? This is also sometimes called the + "can_learn" permission. user: a Django User object (may be an AnonymousUser) diff --git a/openedx/core/djangoapps/xblock/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py index 7934e24bd2..20e6a6dde0 100644 --- a/openedx/core/djangoapps/xblock/rest_api/views.py +++ b/openedx/core/djangoapps/xblock/rest_api/views.py @@ -29,6 +29,7 @@ import openedx.core.djangoapps.site_configuration.helpers as configuration_helpe from openedx.core.djangoapps.xblock.learning_context.manager import get_learning_context_impl from openedx.core.lib.api.view_utils import view_auth_classes from ..api import ( + CheckPerm, get_block_metadata, get_block_display_name, get_handler_url as _get_handler_url, @@ -108,7 +109,7 @@ def embed_block_view(request, usage_key_str, view_name): raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e try: - block = load_block(usage_key, request.user) + block = load_block(usage_key, request.user, check_permission=CheckPerm.CAN_LEARN) except NoSuchUsage as exc: raise NotFound(f"{usage_key} not found") from exc @@ -246,7 +247,8 @@ class BlockFieldsView(APIView): except InvalidKeyError as e: raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e - block = load_block(usage_key, request.user) + # The "fields" view requires "read as author" permissions because the fields can contain answers, etc. + block = load_block(usage_key, request.user, check_permission=CheckPerm.CAN_READ_AS_AUTHOR) block_dict = { "display_name": get_block_display_name(block), # potentially duplicated from metadata "data": block.data, @@ -265,7 +267,7 @@ class BlockFieldsView(APIView): raise NotFound(invalid_not_found_fmt.format(usage_key=usage_key_str)) from e user = request.user - block = load_block(usage_key, user) + block = load_block(usage_key, user, check_permission=CheckPerm.CAN_EDIT) data = request.data.get("data") metadata = request.data.get("metadata")