diff --git a/lms/djangoapps/course_blocks/api.py b/lms/djangoapps/course_blocks/api.py index c978f54e02d5..de4e0443fb08 100644 --- a/lms/djangoapps/course_blocks/api.py +++ b/lms/djangoapps/course_blocks/api.py @@ -108,4 +108,5 @@ def get_course_blocks( transformers, starting_block_usage_key, collected_block_structure, + user, ) diff --git a/lms/djangoapps/course_blocks/tests/test_api.py b/lms/djangoapps/course_blocks/tests/test_api.py new file mode 100644 index 000000000000..52e9c86ec324 --- /dev/null +++ b/lms/djangoapps/course_blocks/tests/test_api.py @@ -0,0 +1,149 @@ +# pylint: disable=attribute-defined-outside-init +""" +Tests for course_blocks API +""" + +from unittest.mock import Mock, patch + +import ddt +from django.http.request import HttpRequest + +from common.djangoapps.student.tests.factories import UserFactory +from lms.djangoapps.course_blocks.api import get_course_blocks +from lms.djangoapps.course_blocks.transformers.tests.helpers import CourseStructureTestCase +from lms.djangoapps.course_blocks.transformers.tests.test_user_partitions import UserPartitionTestMixin +from lms.djangoapps.courseware.block_render import make_track_function, prepare_runtime_for_user +from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers +from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort +from xmodule.modulestore.django import modulestore + + +def get_block_side_effect(block_locator, user_known): + """ + Side effect for `CachingDescriptorSystem.get_block` + """ + store = modulestore() + course = store.get_course(block_locator.course_key) + block = store.get_item(block_locator) + runtime = block.runtime + user = UserFactory.create() + user.known = user_known + + prepare_runtime_for_user( + user=user, + student_data=Mock(), + runtime=runtime, + course_id=block_locator.course_key, + track_function=make_track_function(HttpRequest()), + request_token=Mock(), + course=course, + ) + return block.runtime.get_block_for_descriptor(block) + + +def get_block_side_effect_for_known_user(self, *args, **kwargs): + """ + Side effect for known user test. + """ + return get_block_side_effect(self, True) + + +def get_block_side_effect_for_unknown_user(self, *args, **kwargs): + """ + Side effect for unknown user test. + """ + return get_block_side_effect(self, False) + + +@ddt.ddt +class TestGetCourseBlocks(UserPartitionTestMixin, CourseStructureTestCase): + """ + Tests `get_course_blocks` API + """ + + def setup_partitions_and_course(self): + """ + Setup course structure. + """ + # Set up user partitions and groups. + self.setup_groups_partitions(active=True, num_groups=1) + self.user_partition = self.user_partitions[0] + + # Build course. + self.course_hierarchy = self.get_course_hierarchy() + self.blocks = self.build_course(self.course_hierarchy) + self.course = self.blocks['course'] + + # Set up cohorts. + self.setup_cohorts(self.course) + + def get_course_hierarchy(self): + """ + Returns a course hierarchy to test with. + """ + # course + # / \ + # / \ + # A[0] B + # | + # | + # O + + return [ + { + 'org': 'UserPartitionTransformer', + 'course': 'UP101F', + 'run': 'test_run', + 'user_partitions': [self.user_partition], + '#type': 'course', + '#ref': 'course', + '#children': [ + { + '#type': 'vertical', + '#ref': 'A', + 'metadata': {'group_access': {self.user_partition.id: [0]}}, + }, + {'#type': 'vertical', '#ref': 'B'}, + ], + }, + { + '#type': 'vertical', + '#ref': 'O', + '#parents': ['B'], + }, + ] + + @ddt.data( + (1, ('course', 'B', 'O'), True), + (1, ('course', 'A', 'B', 'O'), False), + (None, ('course', 'B', 'O'), True), + (None, ('course', 'A', 'B', 'O'), False), + ) + @ddt.unpack + def test_get_course_blocks(self, group_id, expected_blocks, user_known): + """ + Tests that `get_course_blocks` returns blocks without access checks for unknown users. + + Access checks are done through the transformers and through Runtime get_block_for_descriptor. Due + to the runtime limitations during the tests, the Runtime access checks are not performed as + get_block_for_descriptor is never called and Block is returned by CachingDescriptorSystem.get_block. + In this test, we mock the CachingDescriptorSystem.get_block and check block access for known and unknown users. + For known users, it performs the Runtime access checks through get_block_for_descriptor. For unknown, it + skips the access checks. + """ + self.setup_partitions_and_course() + if group_id: + cohort = self.partition_cohorts[self.user_partition.id - 1][group_id - 1] + add_user_to_cohort(cohort, self.user.username) + + side_effect = get_block_side_effect_for_known_user if user_known else get_block_side_effect_for_unknown_user + with patch('xmodule.modulestore.split_mongo.split.CachingDescriptorSystem.get_block', side_effect=side_effect): + block_structure = get_course_blocks( + self.user, + self.course.location, + BlockStructureTransformers([]), + ) + self.assertSetEqual( + set(block_structure.get_block_keys()), + self.get_block_key_set(self.blocks, *expected_blocks) + ) diff --git a/openedx/core/djangoapps/content/block_structure/manager.py b/openedx/core/djangoapps/content/block_structure/manager.py index 2f8a041c3163..49f423ce7ac3 100644 --- a/openedx/core/djangoapps/content/block_structure/manager.py +++ b/openedx/core/djangoapps/content/block_structure/manager.py @@ -35,7 +35,7 @@ def __init__(self, root_block_usage_key, modulestore, cache): self.modulestore = modulestore self.store = BlockStructureStore(cache) - def get_transformed(self, transformers, starting_block_usage_key=None, collected_block_structure=None): + def get_transformed(self, transformers, starting_block_usage_key=None, collected_block_structure=None, user=None): """ Returns the transformed Block Structure for the root_block_usage_key, starting at starting_block_usage_key, getting block data from the cache @@ -57,11 +57,14 @@ def get_transformed(self, transformers, starting_block_usage_key=None, collected get_collected. Can be optionally provided if already available, for optimization. + user (django.contrib.auth.models.User) - User object for + which the block structure is to be transformed. + Returns: BlockStructureBlockData - A transformed block structure, starting at starting_block_usage_key. """ - block_structure = collected_block_structure.copy() if collected_block_structure else self.get_collected() + block_structure = collected_block_structure.copy() if collected_block_structure else self.get_collected(user) if starting_block_usage_key: # Override the root_block_usage_key so traversals start at the @@ -77,7 +80,7 @@ def get_transformed(self, transformers, starting_block_usage_key=None, collected transformers.transform(block_structure) return block_structure - def get_collected(self): + def get_collected(self, user=None): """ Returns the collected Block Structure for the root_block_usage_key, getting block data from the cache and modulestore, as needed. @@ -86,6 +89,10 @@ def get_collected(self): the modulestore is accessed if needed (at cache miss), and the transformers data is collected if needed. + In the case of a cache miss, the function bypasses runtime access checks for the current + user. This is done to prevent inconsistencies in the data, which can occur when + certain blocks are inaccessible due to access restrictions. + Returns: BlockStructureBlockData - A collected block structure, starting at root_block_usage_key, with collected data @@ -99,7 +106,15 @@ def get_collected(self): BlockStructureTransformers.verify_versions(block_structure) except (BlockStructureNotFound, TransformerDataIncompatible): - block_structure = self._update_collected() + if user and getattr(user, "known", True): + # This bypasses the runtime access checks. When we are populating the course blocks cache, + # we do not want to perform access checks. Access checks result in inconsistent blocks where + # inaccessible blocks are missing from the cache. Cached course blocks are then used for all the users. + user.known = False + block_structure = self._update_collected() + user.known = True + else: + block_structure = self._update_collected() return block_structure