Merge pull request #35655 from mitodl/asad/bypass-access-checks-when-populating-cache

fix: bypass access checks when populating course blocks cache

Author: pdpinch

lms/djangoapps/course_blocks/api.py

@@ -108,4 +108,5 @@ def get_course_blocks(
         transformers,
         starting_block_usage_key,
         collected_block_structure,
+        user,
     )

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)
+        )

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