From 67d4ace896b6ad25ebc05dbf8c93fa060f0a16b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=B4mulo=20Penido?= Date: Thu, 5 Dec 2024 17:37:10 -0300 Subject: [PATCH] fix: content libraries permissions This PR changes the permissions for content libraries so that only people who can create courses should be allowed to create new content libraries. --- .../rest_api/v1/serializers/home.py | 1 + .../contentstore/rest_api/v1/views/home.py | 1 + .../rest_api/v1/views/tests/test_home.py | 1 + cms/djangoapps/contentstore/utils.py | 8 ++++ .../views/tests/test_course_index.py | 24 ++++++------ .../core/djangoapps/content_libraries/api.py | 7 ++++ .../content_libraries/permissions.py | 15 ++++++-- .../content_libraries/tests/base.py | 2 +- .../tests/test_content_libraries.py | 15 +++++--- .../rest_api/v1/tests/test_views.py | 38 +++++++++---------- 10 files changed, 71 insertions(+), 41 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/home.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/home.py index a81d391b3f69..fa2a651f8a28 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/serializers/home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/home.py @@ -66,6 +66,7 @@ class StudioHomeSerializer(serializers.Serializer): request_course_creator_url = serializers.CharField() rerun_creator_status = serializers.BooleanField() show_new_library_button = serializers.BooleanField() + show_new_library_v2_button = serializers.BooleanField() split_studio_home = serializers.BooleanField() studio_name = serializers.CharField() studio_short_name = serializers.CharField() diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/home.py b/cms/djangoapps/contentstore/rest_api/v1/views/home.py index 06433d9f42d5..3de536d78092 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/home.py @@ -66,6 +66,7 @@ def get(self, request: Request): "request_course_creator_url": "/request_course_creator", "rerun_creator_status": true, "show_new_library_button": true, + "show_new_library_v2_button": true, "split_studio_home": false, "studio_name": "Studio", "studio_short_name": "Studio", diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py index 69eee524373c..d822681abdda 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py @@ -54,6 +54,7 @@ def setUp(self): "request_course_creator_url": "/request_course_creator", "rerun_creator_status": True, "show_new_library_button": True, + "show_new_library_v2_button": True, "split_studio_home": False, "studio_name": settings.STUDIO_NAME, "studio_short_name": settings.STUDIO_SHORT_NAME, diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 964f0c57e757..3f81a860dc59 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1552,6 +1552,9 @@ def get_library_context(request, request_is_json=False): from cms.djangoapps.contentstore.views.library import ( user_can_view_create_library_button, ) + from openedx.core.djangoapps.content_libraries.api import ( + user_can_create_library, + ) libraries = _accessible_libraries_iter(request.user) if libraries_v1_enabled() else [] data = { @@ -1565,6 +1568,7 @@ def get_library_context(request, request_is_json=False): 'courses': [], 'libraries_enabled': libraries_v1_enabled(), 'show_new_library_button': user_can_view_create_library_button(request.user) and request.user.is_active, + 'show_new_library_v2_button': user_can_create_library(request.user), 'user': request.user, 'request_course_creator_url': reverse('request_course_creator'), 'course_creator_status': _get_course_creator_status(request.user), @@ -1686,6 +1690,9 @@ def get_home_context(request, no_course=False): from cms.djangoapps.contentstore.views.library import ( user_can_view_create_library_button, ) + from openedx.core.djangoapps.content_libraries.api import ( + user_can_create_library, + ) active_courses = [] archived_courses = [] @@ -1714,6 +1721,7 @@ def get_home_context(request, no_course=False): 'taxonomy_list_mfe_url': get_taxonomy_list_url(), 'libraries': libraries, 'show_new_library_button': user_can_view_create_library_button(user), + 'show_new_library_v2_button': user_can_create_library(user), 'user': user, 'request_course_creator_url': reverse('request_course_creator'), 'course_creator_status': _get_course_creator_status(user), diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index c3dcfe5305b7..a49273928062 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -433,13 +433,13 @@ def check_index_page(self, separate_archived_courses, org): @override_settings(FEATURES=FEATURES_WITHOUT_HOME_PAGE_COURSE_V2_API) @ddt.data( # Staff user has course staff access - (True, 'staff', None, 0, 21), - (False, 'staff', None, 0, 21), + (True, 'staff', None, 0, 23), + (False, 'staff', None, 0, 23), # Base user has global staff access - (True, 'user', ORG, 2, 21), - (False, 'user', ORG, 2, 21), - (True, 'user', None, 2, 21), - (False, 'user', None, 2, 21), + (True, 'user', ORG, 2, 23), + (False, 'user', ORG, 2, 23), + (True, 'user', None, 2, 23), + (False, 'user', None, 2, 23), ) @ddt.unpack def test_separate_archived_courses(self, separate_archived_courses, username, org, mongo_queries, sql_queries): @@ -464,13 +464,13 @@ def test_separate_archived_courses(self, separate_archived_courses, username, or @override_settings(FEATURES=FEATURES_WITH_HOME_PAGE_COURSE_V2_API) @ddt.data( # Staff user has course staff access - (True, 'staff', None, 0, 21), - (False, 'staff', None, 0, 21), + (True, 'staff', None, 0, 23), + (False, 'staff', None, 0, 23), # Base user has global staff access - (True, 'user', ORG, 0, 21), - (False, 'user', ORG, 0, 21), - (True, 'user', None, 0, 21), - (False, 'user', None, 0, 21), + (True, 'user', ORG, 0, 23), + (False, 'user', ORG, 0, 23), + (True, 'user', None, 0, 23), + (False, 'user', None, 0, 23), ) @ddt.unpack def test_separate_archived_courses_with_home_page_course_v2_api( diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 123e711ab809..6b1660262510 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -308,6 +308,13 @@ class LibraryXBlockType: # ============ +def user_can_create_library(user: AbstractUser) -> bool: + """ + Check if the user has permission to create a content library. + """ + return user.has_perm(permissions.CAN_CREATE_CONTENT_LIBRARY) + + def get_libraries_for_user(user, org=None, library_type=None, text_search=None, order=None): """ Return content libraries that the user has permission to view. diff --git a/openedx/core/djangoapps/content_libraries/permissions.py b/openedx/core/djangoapps/content_libraries/permissions.py index 17671b5659f8..4e72381986ed 100644 --- a/openedx/core/djangoapps/content_libraries/permissions.py +++ b/openedx/core/djangoapps/content_libraries/permissions.py @@ -48,6 +48,12 @@ def is_studio_request(_): return settings.SERVICE_VARIANT == "cms" +@blanket_rule +def is_course_creator(user): + from cms.djangoapps.course_creators.views import get_course_creator_status + + return get_course_creator_status(user) == 'granted' + ########################### Permissions ########################### # Is the user allowed to view XBlocks from the specified content library @@ -68,7 +74,10 @@ def is_studio_request(_): # Is the user allowed to create content libraries? CAN_CREATE_CONTENT_LIBRARY = 'content_libraries.create_library' -perms[CAN_CREATE_CONTENT_LIBRARY] = is_user_active +if settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + perms[CAN_CREATE_CONTENT_LIBRARY] = is_global_staff | (is_user_active & is_course_creator) +else: + perms[CAN_CREATE_CONTENT_LIBRARY] = is_global_staff # Is the user allowed to view the specified content library in Studio, # including to view the raw OLX and asset files? @@ -76,8 +85,8 @@ def is_studio_request(_): perms[CAN_VIEW_THIS_CONTENT_LIBRARY] = is_user_active & ( # Global staff can access any library is_global_staff | - # Some libraries allow anyone to view them in Studio: - Attribute('allow_public_read', True) | + # Libraries with "public read" permissions can be accessed only by course creators + (Attribute('allow_public_read', True) & is_course_creator) | # Otherwise the user must be part of the library's team has_explicit_read_permission_for_library ) diff --git a/openedx/core/djangoapps/content_libraries/tests/base.py b/openedx/core/djangoapps/content_libraries/tests/base.py index 33d5477da698..1756f72ef74c 100644 --- a/openedx/core/djangoapps/content_libraries/tests/base.py +++ b/openedx/core/djangoapps/content_libraries/tests/base.py @@ -71,7 +71,7 @@ class ContentLibrariesRestApiTest(APITransactionTestCase): def setUp(self): super().setUp() - self.user = UserFactory.create(username="Bob", email="bob@example.com", password="edx") + self.user = UserFactory.create(username="Bob", email="bob@example.com", password="edx", is_staff=True) # Create an organization self.organization, _ = Organization.objects.get_or_create( short_name="CL-TEST", 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 2f670d8880a7..34eb7c4f34fc 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -264,6 +264,8 @@ def test_library_blocks(self): # pylint: disable=too-many-statements Tests with some non-ASCII chars in slugs, titles, descriptions. """ + admin = UserFactory.create(username="Admin", email="admin@example.com", is_staff=True) + lib = self._create_library(slug="téstlꜟط", title="A Tést Lꜟطrary", description="Tésting XBlocks") lib_id = lib["id"] assert lib['has_unpublished_changes'] is False @@ -557,7 +559,7 @@ def test_library_permissions(self): # pylint: disable=too-many-statements Learning Core data models. """ # Create a few users to use for all of these tests: - admin = UserFactory.create(username="Admin", email="admin@example.com") + admin = UserFactory.create(username="Admin", email="admin@example.com", is_staff=True) author = UserFactory.create(username="Author", email="author@example.com") reader = UserFactory.create(username="Reader", email="reader@example.com") group = Group.objects.create(name="group1") @@ -679,14 +681,15 @@ def test_library_permissions(self): # pylint: disable=too-many-statements self._get_library_block_asset(block3_key, file_name="static/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: + # Even if we grant allow_public_read, then they can't: with self.as_user(admin): self._update_library(lib_id, allow_public_read=True) self._set_library_block_asset(block3_key, "static/whatever.png", b"data") with self.as_user(random_user): - self._get_library_block_olx(block3_key) + self._get_library_block_olx(block3_key, expect_response=403) + self._get_library_block_fields(block3_key, expect_response=403) + # But he can preview the block: 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") @@ -728,7 +731,7 @@ def test_no_lockout(self): """ Test that administrators cannot be removed if they are the only administrator granted access. """ - admin = UserFactory.create(username="Admin", email="admin@example.com") + admin = UserFactory.create(username="Admin", email="admin@example.com", is_staff=True) successor = UserFactory.create(username="Successor", email="successor@example.com") with self.as_user(admin): lib = self._create_library(slug="permtest", title="Permission Test Library", description="Testing") @@ -1070,7 +1073,7 @@ def test_library_paste_clipboard(self): from openedx.core.djangoapps.content_staging.api import save_xblock_to_user_clipboard # Create user to perform tests on - author = UserFactory.create(username="Author", email="author@example.com") + author = UserFactory.create(username="Author", email="author@example.com", is_staff=True) with self.as_user(author): lib = self._create_library( slug="test_lib_paste_clipboard", diff --git a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py index 463ea42d08e9..5a99868ece66 100644 --- a/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py +++ b/openedx/core/djangoapps/content_tagging/rest_api/v1/tests/test_views.py @@ -13,6 +13,7 @@ import ddt from django.contrib.auth import get_user_model from django.core.files.uploadedfile import SimpleUploadedFile +from edx_django_utils.cache import RequestCache from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator, LibraryCollectionLocator from openedx_tagging.core.tagging.models import Tag, Taxonomy from openedx_tagging.core.tagging.models.system_defined import SystemDefinedTaxonomy @@ -34,7 +35,6 @@ from openedx.core.djangoapps.content_libraries.api import AccessLevel, create_library, set_library_user_permissions from openedx.core.djangoapps.content_tagging import api as tagging_api from openedx.core.djangoapps.content_tagging.models import TaxonomyOrg -from openedx.core.djangoapps.content_tagging.utils import rules_cache from openedx.core.djangolib.testing.utils import skip_unless_cms from ....tests.test_objecttag_export_helpers import TaggedCourseMixin @@ -289,8 +289,8 @@ def setUp(self): self._setUp_taxonomies() self._setUp_collection() - # Clear the rules cache in between test runs to keep query counts consistent. - rules_cache.clear() + # Clear all request caches in between test runs to keep query counts consistent. + RequestCache.clear_all_namespaces() @skip_unless_cms @@ -510,12 +510,12 @@ def test_create_taxonomy(self, user_attr: str, expected_status: int) -> None: @ddt.data( ('staff', 11), - ("content_creatorA", 16), - ("library_staffA", 16), - ("library_userA", 16), - ("instructorA", 16), - ("course_instructorA", 16), - ("course_staffA", 16), + ("content_creatorA", 17), + ("library_staffA", 17), + ("library_userA", 17), + ("instructorA", 17), + ("course_instructorA", 17), + ("course_staffA", 17), ) @ddt.unpack def test_list_taxonomy_query_count(self, user_attr: str, expected_queries: int): @@ -1879,16 +1879,16 @@ def test_get_copied_tags(self): ('staff', 'courseA', 8), ('staff', 'libraryA', 8), ('staff', 'collection_key', 8), - ("content_creatorA", 'courseA', 11, False), - ("content_creatorA", 'libraryA', 11, False), - ("content_creatorA", 'collection_key', 11, False), - ("library_staffA", 'libraryA', 11, False), # Library users can only view objecttags, not change them? - ("library_staffA", 'collection_key', 11, False), - ("library_userA", 'libraryA', 11, False), - ("library_userA", 'collection_key', 11, False), - ("instructorA", 'courseA', 11), - ("course_instructorA", 'courseA', 11), - ("course_staffA", 'courseA', 11), + ("content_creatorA", 'courseA', 12, False), + ("content_creatorA", 'libraryA', 12, False), + ("content_creatorA", 'collection_key', 12, False), + ("library_staffA", 'libraryA', 12, False), # Library users can only view objecttags, not change them? + ("library_staffA", 'collection_key', 12, False), + ("library_userA", 'libraryA', 12, False), + ("library_userA", 'collection_key', 12, False), + ("instructorA", 'courseA', 12), + ("course_instructorA", 'courseA', 12), + ("course_staffA", 'courseA', 12), ) @ddt.unpack def test_object_tags_query_count(