From c609451d2e97b15a30cf1a677cadcbba770b99a6 Mon Sep 17 00:00:00 2001 From: Sid Verma Date: Tue, 14 Jul 2020 01:19:47 +0530 Subject: [PATCH] [BD-14] Limit number of blocks allowed in content libraries (#24276) * Enforce limit on number of blocks allowed in library (blockstore) * Enforce limit on number of blocks allowed in library (modulestore) * Changes from review feedback --- cms/djangoapps/contentstore/views/item.py | 35 +++++++++++++++++++ .../contentstore/views/tests/test_library.py | 19 ++++++++++ cms/envs/common.py | 3 ++ cms/envs/production.py | 3 ++ lms/envs/common.py | 3 ++ lms/envs/production.py | 3 ++ .../core/djangoapps/content_libraries/api.py | 13 +++++++ .../tests/test_content_libraries.py | 16 +++++++++ 8 files changed, 95 insertions(+) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index f6d5824bcd..f9244860e3 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -103,6 +103,19 @@ def _filter_entrance_exam_grader(graders): return graders +def _is_library_component_limit_reached(usage_key): + """ + Verify if the library has reached the maximum number of components allowed in it + """ + store = modulestore() + parent = store.get_item(usage_key) + if not parent.has_children: + # Limit cannot be applied on such items + return False + total_children = len(parent.children) + return total_children + 1 > settings.MAX_BLOCKS_PER_CONTENT_LIBRARY + + @require_http_methods(("DELETE", "GET", "PUT", "POST", "PATCH")) @login_required @expect_json @@ -215,6 +228,18 @@ def xblock_handler(request, usage_key_string): ): raise PermissionDenied() + # Libraries have a maximum component limit enforced on them + if (isinstance(parent_usage_key, LibraryUsageLocator) and + _is_library_component_limit_reached(parent_usage_key)): + return JsonResponse( + { + 'error': _(u'Libraries cannot have more than {limit} components').format( + limit=settings.MAX_BLOCKS_PER_CONTENT_LIBRARY + ) + }, + status=400 + ) + dest_usage_key = _duplicate_item( parent_usage_key, duplicate_source_usage_key, @@ -691,6 +716,16 @@ def _create_item(request): u"Category '%s' not supported for Libraries" % category, content_type='text/plain' ) + if _is_library_component_limit_reached(usage_key): + return JsonResponse( + { + 'error': _(u'Libraries cannot have more than {limit} components').format( + limit=settings.MAX_BLOCKS_PER_CONTENT_LIBRARY + ) + }, + status=400 + ) + created_block = create_xblock( parent_locator=parent_locator, user=request.user, diff --git a/cms/djangoapps/contentstore/views/tests/test_library.py b/cms/djangoapps/contentstore/views/tests/test_library.py index 2344761500..171068ec19 100644 --- a/cms/djangoapps/contentstore/views/tests/test_library.py +++ b/cms/djangoapps/contentstore/views/tests/test_library.py @@ -8,6 +8,7 @@ More important high-level tests are in contentstore/tests/test_libraries.py import ddt import mock from django.conf import settings +from django.urls import reverse from mock import patch from opaque_keys.edx.locator import CourseKey, LibraryLocator from six import binary_type, text_type @@ -342,3 +343,21 @@ class UnitTestLibraries(CourseTestCase): response = self.client.get(manage_users_url) self.assertEqual(response.status_code, 200) self.assertContains(response, extra_user.username) + + def test_component_limits(self): + """ + Test that component limits in libraries are respected. + """ + with self.settings(MAX_BLOCKS_PER_CONTENT_LIBRARY=1): + library = LibraryFactory.create() + data = { + 'parent_locator': str(library.location), + 'category': 'html' + } + response = self.client.ajax_post(reverse('xblock_handler'), data) + self.assertEqual(response.status_code, 200) + + # Adding another component should cause failure: + response = self.client.ajax_post(reverse('xblock_handler'), data) + self.assertEqual(response.status_code, 400) + self.assertIn('cannot have more than 1 component', parse_json(response)['error']) diff --git a/cms/envs/common.py b/cms/envs/common.py index c495596554..bf13fa14fa 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -2261,3 +2261,6 @@ LOGISTRATION_RATELIMIT_RATE = '100/5m' ##### PASSWORD RESET RATE LIMIT SETTINGS ##### PASSWORD_RESET_IP_RATE = '1/m' PASSWORD_RESET_EMAIL_RATE = '2/h' + +######################## Setting for content libraries ######################## +MAX_BLOCKS_PER_CONTENT_LIBRARY = 1000 diff --git a/cms/envs/production.py b/cms/envs/production.py index 45850fa03e..0882aeef48 100644 --- a/cms/envs/production.py +++ b/cms/envs/production.py @@ -531,6 +531,9 @@ RETIREMENT_SERVICE_WORKER_USERNAME = ENV_TOKENS.get( ############### Settings for edx-rbac ############### SYSTEM_WIDE_ROLE_CLASSES = ENV_TOKENS.get('SYSTEM_WIDE_ROLE_CLASSES') or SYSTEM_WIDE_ROLE_CLASSES +######################## Setting for content libraries ######################## +MAX_BLOCKS_PER_CONTENT_LIBRARY = ENV_TOKENS.get('MAX_BLOCKS_PER_CONTENT_LIBRARY', MAX_BLOCKS_PER_CONTENT_LIBRARY) + ####################### Plugin Settings ########################## # This is at the bottom because it is going to load more settings after base settings are loaded diff --git a/lms/envs/common.py b/lms/envs/common.py index 6792b1398f..a34575ef50 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3931,3 +3931,6 @@ GITHUB_REPO_ROOT = '/edx/var/edxapp/data' ##################### SUPPORT URL ############################ SUPPORT_HOW_TO_UNENROLL_LINK = '' + +######################## Setting for content libraries ######################## +MAX_BLOCKS_PER_CONTENT_LIBRARY = 1000 diff --git a/lms/envs/production.py b/lms/envs/production.py index 21f5c494a8..239d880941 100644 --- a/lms/envs/production.py +++ b/lms/envs/production.py @@ -936,6 +936,9 @@ MAINTENANCE_BANNER_TEXT = ENV_TOKENS.get('MAINTENANCE_BANNER_TEXT', None) ########################## limiting dashboard courses ###################### DASHBOARD_COURSE_LIMIT = ENV_TOKENS.get('DASHBOARD_COURSE_LIMIT', None) +######################## Setting for content libraries ######################## +MAX_BLOCKS_PER_CONTENT_LIBRARY = ENV_TOKENS.get('MAX_BLOCKS_PER_CONTENT_LIBRARY', MAX_BLOCKS_PER_CONTENT_LIBRARY) + ############################### Plugin Settings ############################### # This is at the bottom because it is going to load more settings after base settings are loaded diff --git a/openedx/core/djangoapps/content_libraries/api.py b/openedx/core/djangoapps/content_libraries/api.py index 0366a87d46..411b302d46 100644 --- a/openedx/core/djangoapps/content_libraries/api.py +++ b/openedx/core/djangoapps/content_libraries/api.py @@ -39,10 +39,12 @@ from uuid import UUID import logging import attr +from django.conf import settings from django.contrib.auth.models import AbstractUser, Group from django.core.exceptions import PermissionDenied from django.core.validators import validate_unicode_slug from django.db import IntegrityError +from django.utils.translation import ugettext as _ from lxml import etree from opaque_keys.edx.keys import LearningContextKey from opaque_keys.edx.locator import BundleDefinitionLocator, LibraryLocatorV2, LibraryUsageLocatorV2 @@ -95,6 +97,10 @@ class LibraryBlockAlreadyExists(KeyError): """ An XBlock with that ID already exists in the library """ +class BlockLimitReachedError(Exception): + """ Maximum number of allowed XBlocks in the library reached """ + + class InvalidNameError(ValueError): """ The specified name/identifier is not valid """ @@ -513,6 +519,13 @@ def create_library_block(library_key, block_type, definition_id): """ assert isinstance(library_key, LibraryLocatorV2) ref = ContentLibrary.objects.get_by_key(library_key) + lib_bundle = LibraryBundle(library_key, ref.bundle_uuid, draft_name=DRAFT_NAME) + # Total number of blocks should not exceed the maximum allowed + total_blocks = len(lib_bundle.get_top_level_usages()) + if total_blocks + 1 > settings.MAX_BLOCKS_PER_CONTENT_LIBRARY: + raise BlockLimitReachedError( + _(u"Library cannot have more than {} XBlocks").format(settings.MAX_BLOCKS_PER_CONTENT_LIBRARY) + ) # Make sure the proposed ID will be valid: validate_unicode_slug(definition_id) # Ensure the XBlock type is valid and installed: 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 afa54fd129..4cc695a1ad 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -8,6 +8,7 @@ from uuid import UUID from django.contrib.auth.models import Group from openedx.core.djangoapps.content_libraries.tests.base import ContentLibrariesRestApiTest +from openedx.core.djangoapps.content_libraries.api import BlockLimitReachedError from student.tests.factories import UserFactory @@ -443,3 +444,18 @@ class ContentLibrariesTest(ContentLibrariesRestApiTest): self.assertEqual(links_created[1]["version"], 1) self.assertEqual(links_created[1]["latest_version"], 2) self.assertEqual(links_created[1]["opaque_key"], bank_lib_id) + + def test_library_blocks_limit(self): + """ + Test that libraries don't allow more than specified blocks + """ + with self.settings(MAX_BLOCKS_PER_CONTENT_LIBRARY=1): + lib = self._create_library(slug="test_lib_limits", title="Limits Test Library", description="Testing XBlocks limits in a library") + lib_id = lib["id"] + block_data = self._add_block_to_library(lib_id, "unit", "unit1") + # Second block should throw error + with self.assertRaises(BlockLimitReachedError): + self._add_block_to_library(lib_id, "problem", "problem1") + # Also check that limit applies to child blocks too + with self.assertRaises(BlockLimitReachedError): + self._add_block_to_library(lib_id, "html", "html1", parent_block=block_data["id"])