Merge branch 'master' into jipm/add_tenant_aware_filter

openedx · Oct 9, 2024 · 8e60d45 · 8e60d45
2 parents 98159aa + f5b8839
commit 8e60d45
Show file tree

Hide file tree

Showing 13 changed files with 304 additions and 168 deletions.
diff --git a/...ates/content_libraries/xblock_iframe.html → ...on/templates/xblock_v2/xblock_iframe.html b/...ates/content_libraries/xblock_iframe.html → ...on/templates/xblock_v2/xblock_iframe.html
@@ -156,7 +156,7 @@
 <!-- The default stylesheet will set the body min-height to 100% (a common strategy to allow for background
   images to fill the viewport), but this has the undesireable side-effect of causing an infinite loop via the onResize
   event listeners below, in certain situations.  Resetting it to the default "auto" skirts the problem.-->
-<body style="min-height: auto">
+<body style="min-height: auto; background-color: white;">
   <!-- fragment body -->
   {{ fragment.body_html | safe }}
   <!-- fragment foot -->

diff --git a/lms/djangoapps/discussion/rest_api/discussions_notifications.py b/lms/djangoapps/discussion/rest_api/discussions_notifications.py
@@ -392,7 +392,8 @@ def clean_thread_html_body(html_body):
         "video", "track",  # Video Tags
         "audio",  # Audio Tags
         "embed", "object", "iframe",  # Embedded Content
-        "script"
+        "script",
+        "b", "strong", "i", "em", "u", "s", "strike", "del", "ins", "mark", "sub", "sup",  # Text Formatting
     ]
 
     # Remove the specified tags while keeping their content
@@ -403,9 +404,10 @@ def clean_thread_html_body(html_body):
     # Replace tags that are not allowed in email
     tags_to_update = [
         {"source": "button", "target": "span"},
-        {"source": "h1", "target": "h4"},
-        {"source": "h2", "target": "h4"},
-        {"source": "h3", "target": "h4"},
+        *[
+            {"source": tag, "target": "p"}
+            for tag in ["div", "section", "article", "h1", "h2", "h3", "h4", "h5", "h6"]
+        ],
     ]
     for tag_dict in tags_to_update:
         for source_tag in html_body.find_all(tag_dict['source']):
@@ -414,4 +416,7 @@ def clean_thread_html_body(html_body):
                 target_tag.string = source_tag.string
             source_tag.replace_with(target_tag)
 
+    for tag in html_body.find_all(True):
+        tag.attrs = {}
+        tag['style'] = 'margin: 0'
     return str(html_body)
diff --git a/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py b/lms/djangoapps/discussion/rest_api/tests/test_discussions_notifications.py
@@ -104,14 +104,14 @@ def test_html_tags_removal(self):
         <p>This is a <a href="#">link</a> to a page.</p>
         <p>Here is an image: <img src="image.jpg" alt="image"></p>
         <p>Embedded video: <iframe src="video.mp4"></iframe></p>
-        <p>Script test: <script>alert('hello');</script></p>
+        <p>Script test: <script>alert("hello");</script></p>
         <p>Some other content that should remain.</p>
         """
-        expected_output = ("<p>This is a link to a page.</p>"
-                           "<p>Here is an image: </p>"
-                           "<p>Embedded video: </p>"
-                           "<p>Script test: alert('hello');</p>"
-                           "<p>Some other content that should remain.</p>")
+        expected_output = ('<p style="margin: 0">This is a link to a page.</p>'
+                           '<p style="margin: 0">Here is an image: </p>'
+                           '<p style="margin: 0">Embedded video: </p>'
+                           '<p style="margin: 0">Script test: alert("hello");</p>'
+                           '<p style="margin: 0">Some other content that should remain.</p>')
 
         result = clean_thread_html_body(html_body)
 
@@ -132,19 +132,16 @@ def test_truncate_html_body(self):
         """
         Test that the clean_thread_html_body function truncates the HTML body to 500 characters
         """
-        html_body = """
-        <p>This is a long text that should be truncated to 500 characters.</p>
-        """ * 20  # Repeat to exceed 500 characters
-
-        result = clean_thread_html_body(html_body)
-        self.assertGreaterEqual(500, len(result))
+        html_body = "This is a long text that should be truncated to 500 characters." * 20
+        result = clean_thread_html_body(f"<p>{html_body}</p>")
+        self.assertGreaterEqual(525, len(result))    # 500 characters + 25 characters for the HTML tags
 
     def test_no_tags_to_remove(self):
         """
         Test that the clean_thread_html_body function does not remove any tags if there are no unwanted tags
         """
         html_body = "<p>This paragraph has no tags to remove.</p>"
-        expected_output = "<p>This paragraph has no tags to remove.</p>"
+        expected_output = '<p style="margin: 0">This paragraph has no tags to remove.</p>'
 
         result = clean_thread_html_body(html_body)
         self.assertEqual(result, expected_output)
@@ -169,28 +166,33 @@ def test_only_script_tag(self):
         result = clean_thread_html_body(html_body)
         self.assertEqual(result.strip(), expected_output)
 
+    def test_tag_replace(self):
+        """
+        Tests if the clean_thread_html_body function replaces tags
+        """
+        for tag in ["div", "section", "article", "h1", "h2", "h3", "h4", "h5", "h6"]:
+            html_body = f'<{tag}>Text</{tag}>'
+            result = clean_thread_html_body(html_body)
+            self.assertEqual(result, '<p style="margin: 0">Text</p>')
+
     def test_button_tag_replace(self):
         """
         Tests that the clean_thread_html_body function replaces the button tag with span tag
         """
         # Tests for button replacement tag with text
         html_body = '<button class="abc">Button</button>'
-        expected_output = '<span class="abc">Button</span>'
+        expected_output = '<span style="margin: 0">Button</span>'
         result = clean_thread_html_body(html_body)
         self.assertEqual(result, expected_output)
 
         # Tests button tag replacement without text
         html_body = '<button class="abc"></button>'
-        expected_output = '<span class="abc"></span>'
+        expected_output = '<span style="margin: 0"></span>'
         result = clean_thread_html_body(html_body)
         self.assertEqual(result, expected_output)
 
-    def test_heading_tag_replace(self):
-        """
-        Tests that the clean_thread_html_body function replaces the h1, h2 and h3 tags with h4 tag
-        """
-        for tag in ['h1', 'h2', 'h3']:
-            html_body = f'<{tag}>Heading</{tag}>'
-            expected_output = '<h4>Heading</h4>'
-            result = clean_thread_html_body(html_body)
-            self.assertEqual(result, expected_output)
+    def test_attributes_removal_from_tag(self):
+        # Tests for removal of attributes from tags
+        html_body = '<p class="abc" style="color:red" aria-disabled=true>Paragraph</p>'
+        result = clean_thread_html_body(html_body)
+        self.assertEqual(result, '<p style="margin: 0">Paragraph</p>')
diff --git a/lms/templates/xblock_v2/xblock_iframe.html b/lms/templates/xblock_v2/xblock_iframe.html
diff --git 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 @@ def __init__(self, **kwargs):
         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)?
-
-        user: a Django User object (may be an AnonymousUser)
+        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)?
 
-        usage_key: the UsageKeyV2 subclass used for this learning context
+        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_EDIT_THIS_CONTENT_LIBRARY)
 
-        Must return a boolean.
+    def can_view_block_for_editing(self, user: UserType, usage_key: UsageKeyV2) -> bool:
         """
-        try:
-            api.require_permission_for_library_key(usage_key.lib_key, user, permissions.CAN_EDIT_THIS_CONTENT_LIBRARY)
-        except (PermissionDenied, api.ContentLibraryNotFound):
-            return False
+        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.block_exists(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, usage_key):
+    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 @@ def block_exists(self, usage_key):
         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 @@ def block_exists(self, usage_key):
             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
@@ -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 @@
 )
 
 
+# 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 @@
 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
@@ -308,3 +308,12 @@ def _get_block_handler_url(self, block_key, handler_name):
         """
         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
@@ -502,6 +502,30 @@ def test_library_blocks_type_constrained(self, slug, library_type, block_type, e
         # 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 @@ def test_library_permissions(self):  # pylint: disable=too-many-statements
         # 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, "<problem/>", expect_response=403)
+                self._set_library_block_fields(block3_key, {"data": "<problem />", "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 @@ def test_library_permissions(self):  # pylint: disable=too-many-statements
         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 @@ def test_xblock_handler_invalid_key(self):
             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.",
-        })