Merge pull request #11331 from CredoReference/invalid-studio-user-permissions

Invalid StudioPermissionsService object in API to show/save xblock settings in CMS

cms/djangoapps/contentstore/tests/test_libraries.py

@@ -23,6 +23,8 @@ from mock import Mock
 from opaque_keys.edx.locator import CourseKey, LibraryLocator
 from openedx.core.djangoapps.content.course_structures.tests import SignalDisconnectTestMixin
 from xblock_django.user_service import DjangoXBlockUserService
+from xmodule.x_module import STUDIO_VIEW
+from student import auth
 
 
 class LibraryTestCase(ModuleStoreTestCase):
@@ -30,16 +32,22 @@ class LibraryTestCase(ModuleStoreTestCase):
     Common functionality for content libraries tests
     """
     def setUp(self):
-        user_password = super(LibraryTestCase, self).setUp()
+        self.user_password = super(LibraryTestCase, self).setUp()
 
         self.client = AjaxEnabledTestClient()
-        self.client.login(username=self.user.username, password=user_password)
+        self._login_as_staff_user(logout_first=False)
 
         self.lib_key = self._create_library()
         self.library = modulestore().get_library(self.lib_key)
 
         self.session_data = {}  # Used by _bind_module
 
+    def _login_as_staff_user(self, logout_first=True):
+        """ Login as a staff user """
+        if logout_first:
+            self.client.logout()
+        self.client.login(username=self.user.username, password=self.user_password)
+
     def _create_library(self, org="org", library="lib", display_name="Test Library"):
         """
         Helper method used to create a library. Uses the REST API.
@@ -729,6 +737,64 @@ class TestLibraryAccess(SignalDisconnectTestMixin, LibraryTestCase):
         lc_block = self._refresh_children(lc_block, status_code_expected=200 if expected_result else 403)
         self.assertEqual(len(lc_block.children), 1 if expected_result else 0)
 
+    def test_studio_user_permissions(self):
+        """
+        Test that user could attach to the problem only libraries that he has access (or which were created by him).
+        This test was created on the basis of bug described in the pull requests on github:
+        https://github.com/edx/edx-platform/pull/11331
+        https://github.com/edx/edx-platform/pull/11611
+        """
+        self._create_library(org='admin_org_1', library='lib_adm_1', display_name='admin_lib_1')
+        self._create_library(org='admin_org_2', library='lib_adm_2', display_name='admin_lib_2')
+
+        self._login_as_non_staff_user()
+
+        self._create_library(org='staff_org_1', library='lib_staff_1', display_name='staff_lib_1')
+        self._create_library(org='staff_org_2', library='lib_staff_2', display_name='staff_lib_2')
+
+        with modulestore().default_store(ModuleStoreEnum.Type.split):
+            course = CourseFactory.create()
+
+        instructor_role = CourseInstructorRole(course.id)
+        auth.add_users(self.user, instructor_role, self.non_staff_user)
+
+        lib_block = ItemFactory.create(
+            category='library_content',
+            parent_location=course.location,
+            user_id=self.non_staff_user.id,
+            publish_item=False
+        )
+
+        def _get_settings_html():
+            """
+            Helper function to get block settings HTML
+            Used to check the available libraries.
+            """
+            edit_view_url = reverse_usage_url("xblock_view_handler", lib_block.location, {"view_name": STUDIO_VIEW})
+
+            resp = self.client.get_json(edit_view_url)
+            self.assertEquals(resp.status_code, 200)
+
+            return parse_json(resp)['html']
+
+        self._login_as_staff_user()
+        staff_settings_html = _get_settings_html()
+        self.assertIn('staff_lib_1', staff_settings_html)
+        self.assertIn('staff_lib_2', staff_settings_html)
+        self.assertIn('admin_lib_1', staff_settings_html)
+        self.assertIn('admin_lib_2', staff_settings_html)
+
+        self._login_as_non_staff_user()
+        response = self.client.get_json(LIBRARY_REST_URL)
+        staff_libs = parse_json(response)
+        self.assertEquals(2, len(staff_libs))
+
+        non_staff_settings_html = _get_settings_html()
+        self.assertIn('staff_lib_1', non_staff_settings_html)
+        self.assertIn('staff_lib_2', non_staff_settings_html)
+        self.assertNotIn('admin_lib_1', non_staff_settings_html)
+        self.assertNotIn('admin_lib_2', non_staff_settings_html)
+
 
 @ddt.ddt
 @override_settings(SEARCH_ENGINE=None)

cms/djangoapps/contentstore/views/component.py

@@ -21,7 +21,7 @@ from xblock.runtime import Mixologist
 
 from contentstore.utils import get_lms_link_for_item
 from contentstore.views.helpers import get_parent_xblock, is_unit, xblock_type_display_name
-from contentstore.views.item import create_xblock_info, add_container_page_publishing_info
+from contentstore.views.item import create_xblock_info, add_container_page_publishing_info, StudioEditModuleRuntime
 
 from opaque_keys.edx.keys import UsageKey
 
@@ -330,6 +330,7 @@ def component_handler(request, usage_key_string, handler, suffix=''):
     usage_key = UsageKey.from_string(usage_key_string)
 
     descriptor = modulestore().get_item(usage_key)
+    descriptor.xmodule_runtime = StudioEditModuleRuntime(request.user)
     # Let the module handle the AJAX
     req = django_to_webob_request(request)
 

cms/djangoapps/contentstore/views/item.py

@@ -21,6 +21,7 @@ from opaque_keys.edx.locator import LibraryUsageLocator
 from pytz import UTC
 from xblock.fields import Scope
 from xblock.fragment import Fragment
+from xblock_django.user_service import DjangoXBlockUserService
 
 from cms.lib.xblock.authoring_mixin import VISIBILITY_VIEW
 from contentstore.utils import (
@@ -51,6 +52,7 @@ from xmodule.modulestore.inheritance import own_metadata
 from xmodule.tabs import CourseTabList
 from xmodule.x_module import PREVIEW_VIEWS, STUDIO_VIEW, STUDENT_VIEW, DEPRECATION_VSCOMPAT_EVENT
 
+
 __all__ = [
     'orphan_handler', 'xblock_handler', 'xblock_view_handler', 'xblock_outline_handler', 'xblock_container_handler'
 ]
@@ -198,6 +200,49 @@ def xblock_handler(request, usage_key_string):
         )
 
 
+class StudioPermissionsService(object):
+    """
+    Service that can provide information about a user's permissions.
+
+    Deprecated. To be replaced by a more general authorization service.
+
+    Only used by LibraryContentDescriptor (and library_tools.py).
+    """
+    def __init__(self, user):
+        self._user = user
+
+    def can_read(self, course_key):
+        """ Does the user have read access to the given course/library? """
+        return has_studio_read_access(self._user, course_key)
+
+    def can_write(self, course_key):
+        """ Does the user have read access to the given course/library? """
+        return has_studio_write_access(self._user, course_key)
+
+
+class StudioEditModuleRuntime(object):
+    """
+    An extremely minimal ModuleSystem shim used for XBlock edits and studio_view.
+    (i.e. whenever we're not using PreviewModuleSystem.) This is required to make information
+    about the current user (especially permissions) available via services as needed.
+    """
+    def __init__(self, user):
+        self._user = user
+
+    def service(self, block, service_name):
+        """
+        This block is not bound to a user but some blocks (LibraryContentModule) may need
+        user-specific services to check for permissions, etc.
+        If we return None here, CombinedSystem will load services from the descriptor runtime.
+        """
+        if block.service_declaration(service_name) is not None:
+            if service_name == "user":
+                return DjangoXBlockUserService(self._user)
+            if service_name == "studio_user_permissions":
+                return StudioPermissionsService(self._user)
+        return None
+
+
 @require_http_methods(("GET"))
 @login_required
 @expect_json
@@ -231,6 +276,9 @@ def xblock_view_handler(request, usage_key_string, view_name):
         ))
 
         if view_name in (STUDIO_VIEW, VISIBILITY_VIEW):
+            if view_name == STUDIO_VIEW and xblock.xmodule_runtime is None:
+                xblock.xmodule_runtime = StudioEditModuleRuntime(request.user)
+
             try:
                 fragment = xblock.render(view_name)
             # catch exceptions indiscriminately, since after this point they escape the
@@ -375,6 +423,7 @@ def _update_with_callback(xblock, user, old_metadata=None, old_content=None):
             old_metadata = own_metadata(xblock)
         if old_content is None:
             old_content = xblock.get_explicitly_set_fields_by_scope(Scope.content)
+        xblock.xmodule_runtime = StudioEditModuleRuntime(user)
         xblock.editor_saved(user, old_metadata, old_content)
 
     # Update after the callback so any changes made in the callback will get persisted.
@@ -624,6 +673,7 @@ def _duplicate_item(parent_usage_key, duplicate_source_usage_key, user, display_
             # Allow an XBlock to do anything fancy it may need to when duplicated from another block.
             # These blocks may handle their own children or parenting if needed. Let them return booleans to
             # let us know if we need to handle these or not.
+            dest_module.xmodule_runtime = StudioEditModuleRuntime(user)
             children_handled = dest_module.studio_post_duplicate(store, source_item)
 
         # Children are not automatically copied over (and not all xblocks have a 'children' attribute).

cms/djangoapps/contentstore/views/preview.py

@@ -16,7 +16,6 @@ from xmodule.x_module import PREVIEW_VIEWS, STUDENT_VIEW, AUTHOR_VIEW
 from xmodule.contentstore.django import contentstore
 from xmodule.error_module import ErrorDescriptor
 from xmodule.exceptions import NotFoundError, ProcessingError
-from xmodule.library_tools import LibraryToolsService
 from xmodule.services import SettingsService
 from xmodule.modulestore.django import modulestore, ModuleI18nService
 from xmodule.mixin import wrap_with_license
@@ -150,28 +149,6 @@ class PreviewModuleSystem(ModuleSystem):  # pylint: disable=abstract-method
         return result
 
 
-class StudioPermissionsService(object):
-    """
-    Service that can provide information about a user's permissions.
-
-    Deprecated. To be replaced by a more general authorization service.
-
-    Only used by LibraryContentDescriptor (and library_tools.py).
-    """
-
-    def __init__(self, request):
-        super(StudioPermissionsService, self).__init__()
-        self._request = request
-
-    def can_read(self, course_key):
-        """ Does the user have read access to the given course/library? """
-        return has_studio_read_access(self._request.user, course_key)
-
-    def can_write(self, course_key):
-        """ Does the user have read access to the given course/library? """
-        return has_studio_write_access(self._request.user, course_key)
-
-
 def _preview_module_system(request, descriptor, field_data):
     """
     Returns a ModuleSystem for the specified descriptor that is specialized for
@@ -213,8 +190,6 @@ def _preview_module_system(request, descriptor, field_data):
         # stick the license wrapper in front
         wrappers.insert(0, wrap_with_license)
 
-    descriptor.runtime._services['studio_user_permissions'] = StudioPermissionsService(request)  # pylint: disable=protected-access
-
     return PreviewModuleSystem(
         static_url=settings.STATIC_URL,
         # TODO (cpennington): Do we want to track how instructors are using the preview problems?
@@ -241,7 +216,6 @@ def _preview_module_system(request, descriptor, field_data):
         services={
             "field-data": field_data,
             "i18n": ModuleI18nService,
-            "library_tools": LibraryToolsService(modulestore()),
             "settings": SettingsService(),
             "user": DjangoXBlockUserService(request.user),
         },

common/lib/xmodule/xmodule/library_content_module.py

@@ -576,12 +576,10 @@ class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDe
         """
         lib_tools = self.runtime.service(self, 'library_tools')
         user_perms = self.runtime.service(self, 'studio_user_permissions')
-        all_libraries = lib_tools.list_available_libraries()
-        if user_perms:
-            all_libraries = [
-                (key, name) for key, name in all_libraries
-                if user_perms.can_read(key) or self.source_library_id == unicode(key)
-            ]
+        all_libraries = [
+            (key, name) for key, name in lib_tools.list_available_libraries()
+            if user_perms.can_read(key) or self.source_library_id == unicode(key)
+        ]
         all_libraries.sort(key=lambda entry: entry[1])  # Sort by name
         if self.source_library_id and self.source_library_key not in [entry[0] for entry in all_libraries]:
             all_libraries.append((self.source_library_id, _(u"Invalid Library")))

common/lib/xmodule/xmodule/x_module.py

@@ -1482,8 +1482,9 @@ class DescriptorSystem(MetricsMixin, ConfigurableFragmentWrapper, Runtime):
         """
         potential_set = set(super(DescriptorSystem, self).applicable_aside_types(block))
         if getattr(block, 'xmodule_runtime', None) is not None:
-            application_set = set(block.xmodule_runtime.applicable_aside_types(block))
-            return list(potential_set.intersection(application_set))
+            if hasattr(block.xmodule_runtime, 'applicable_aside_types'):
+                application_set = set(block.xmodule_runtime.applicable_aside_types(block))
+                return list(potential_set.intersection(application_set))
         return list(potential_set)
 
     def resource_url(self, resource):