From 5a149f0fe314ae433b3a32011f319307dc44175a Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 5 Jan 2021 13:17:42 -0500 Subject: [PATCH 1/2] Enable timed/special exams and pre-reqs in devstack. These are common and useful enough features that it makes sense to enable it for developers by default. --- cms/envs/devstack.py | 4 ++++ lms/envs/devstack.py | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index e1080fd924..7969d335b7 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -236,3 +236,7 @@ CORS_ORIGIN_ALLOW_ALL = True CORS_ALLOW_HEADERS = corsheaders_default_headers + ( 'use-jwt-cookie', ) + +################### Special Exams (Proctoring) and Prereqs ################### +FEATURES['ENABLE_SPECIAL_EXAMS'] = True +FEATURES['ENABLE_PREREQUISITE_COURSES'] = True diff --git a/lms/envs/devstack.py b/lms/envs/devstack.py index 3a0f04fb7d..a0f4907cd1 100644 --- a/lms/envs/devstack.py +++ b/lms/envs/devstack.py @@ -427,3 +427,7 @@ if os.path.isfile(join(dirname(abspath(__file__)), 'private.py')): # Uncomment the lines below if you'd like to see SQL statements in your devstack LMS log. # LOGGING['handlers']['console']['level'] = 'DEBUG' # LOGGING['loggers']['django.db.backends'] = {'handlers': ['console'], 'level': 'DEBUG', 'propagate': False} + +################### Special Exams (Proctoring) and Prereqs ################### +FEATURES['ENABLE_SPECIAL_EXAMS'] = True +FEATURES['ENABLE_PREREQUISITE_COURSES'] = True From 5f94a082ce1141d9214e83c152baa00186278ad3 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Tue, 5 Jan 2021 13:18:39 -0500 Subject: [PATCH 2/2] Check sequence-level gating in render_xblock (TNL-7636). There is certain gating logic around pre-reqs, timed exams, etc. that happen at the SequenceModule level, and should be respected when rendering descendant XBlocks (like individual problems) that are in that Sequence. Rather than do a risky refactoring, I'm keeping that logic where it is and having the render_xblock view climb up through the ancestor list to call the SequenceModule for that gating information. We do _not_ check all descendants (so cousin leaf nodes in the sequence) for cotent-type-based restrictions because sequences can become very large (esp. when content libraries are used), and there is a performance overhead. If the enclosing sequence is gated in some way, we redirect to the render_xblock view for that sequence, where hopefully some useful messaging will be available. This is a stopgap. That redirect should never happen because we should never be calling the leaf XBlock for a sequence that is restricted in the MFE. But if somehow we get there anyway, either by bug or by intrepid user fiddling, it's better to redirect somewhere that an error _might_ be surfaced rather than just failing. This will actually be a little overzealous and lock things down that should be made visible later. If there's a timed exam and the exam is completed, it should be the case that content is visible (just read-only). This commit will block the content before the exam starts (this is right), open the content while the exam is live (this is right), but make the content unavailable after the exam period has finished (this is wrong). But I am going to go forward with this even knowing it's wrong because: 1. The render_xblock endpoint should never currently be used in timed exams in an intentional way. Neither the mobile experience nor the courseware MFE support it. 2. This fix will address security concerns for creative access patterns, even if it goes too far. 3. We're going to need to do a lot of work to address both pluggable access permissions handling and special exams in the courseware MFE, and a better implementation can be done then. 4. I've had multiple failed attempts to get this to work without breaking things on and off over the course of weeks, and this is a relatively low risk way of doing it that doesn't involve a major refactoring (though the bill for that will come due when we bring timed exams to the MFE). --- common/lib/xmodule/xmodule/seq_module.py | 44 ++++++++++++ lms/djangoapps/courseware/tests/test_views.py | 49 ++++++++++++- lms/djangoapps/courseware/views/views.py | 70 +++++++++++++++++++ 3 files changed, 162 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index 2f23b2cd9e..87ff101cf2 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/common/lib/xmodule/xmodule/seq_module.py @@ -560,6 +560,50 @@ class SequenceModule(SequenceFields, ProctoringFields, XModule): return None + def descendants_are_gated(self): + """ + Sequences do their own access gating logic as to whether their content + should be viewable, based on things like pre-reqs and time exam starts. + Ideally, this information would be passed down to all descendants so + that they would know if it's safe to render themselves, but the least + invasive patch to this is to make a method that rendering Django views + can use to verify before rendering descendants. + + This does _NOT_ check for the content types of children because the + performing that traversal undoes a lot of the performance gains made in + large sequences when hitting the render_xblock endpoint directly. This + method is here mostly to help render_xblock figure out if it's okay to + render a descendant of a sequence to guard against malicious actors. So + the "let's check all descendants to not let people start an exam they + can't finish" reasoning of doing the full traversal does not apply. + + Returns: + True if this sequence and its descendants are gated by what are + currently sequence-level checks. + False if the sequence is and its decendants are not gated. + + Note that this gating logic is only a part of the equation when it + comes to determining whether a student is allowed to access this, + with other checks being done in has_access calls. + """ + if self.runtime.user_is_staff: + return False + + # We're not allowed to see it because of pre-reqs that haven't been + # fullfilled. + if self._required_prereq(): + prereq_met, _prereq_meta_info = self._compute_is_prereq_met(True) + if not prereq_met: + return True + + # Are we a time limited test that hasn't started yet? + if self.is_time_limited: + if self._time_limited_student_view() or self._hidden_content_student_view({}): + return True + + # Otherwise, nothing is blocking us. + return False + def _compute_is_prereq_met(self, recalc_on_unmet): """ Evaluate if the user has completed the prerequisite diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 173c00f288..6bd3cde360 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -2878,7 +2878,7 @@ class TestRenderXBlock(RenderXBlockTestMixin, ModuleStoreTestCase, CompletionWaf """ Overridable method to get the response from the endpoint that is being tested. """ - url = reverse('render_xblock', kwargs={'usage_key_string': six.text_type(usage_key)}) + url = reverse('render_xblock', kwargs={'usage_key_string': str(usage_key)}) if url_encoded_params: url += '?' + url_encoded_params return self.client.get(url) @@ -2934,6 +2934,53 @@ class TestRenderXBlock(RenderXBlockTestMixin, ModuleStoreTestCase, CompletionWaf self.assertContains(response, 'data-enable-completion-on-view-service="false"') self.assertNotContains(response, 'data-mark-completed-on-view-after-delay') + def test_rendering_descendant_of_gated_sequence(self): + """ + Test that we redirect instead of rendering what should be gated content, + for things that are gated at the sequence level. + """ + with self.store.default_store(ModuleStoreEnum.Type.split): + # pylint:disable=attribute-defined-outside-init + self.course = CourseFactory.create(**self.course_options()) + self.chapter = ItemFactory.create(parent=self.course, category='chapter') + self.sequence = ItemFactory.create( + parent=self.chapter, + category='sequential', + display_name='Sequence', + is_time_limited=True, + ) + self.vertical_block = ItemFactory.create( + parent=self.sequence, + category='vertical', + display_name="Vertical", + ) + self.html_block = ItemFactory.create( + parent=self.vertical_block, + category='html', + data="

Test HTML Content

" + ) + self.problem_block = ItemFactory.create( + parent=self.vertical_block, + category='problem', + display_name='Problem' + ) + CourseOverview.load_from_module_store(self.course.id) + self.setup_user(admin=False, enroll=True, login=True) + + # Problem and Vertical response should both redirect to the Sequential + # (where useful messaging would be). + seq_url = reverse('render_xblock', kwargs={'usage_key_string': str(self.sequence.location)}) + for block in [self.problem_block, self.vertical_block]: + response = self.get_response(usage_key=block.location) + self.assertEqual(response.status_code, 302) + self.assertEqual(response.get('Location'), seq_url) + + # The Sequence itself 200s (or we risk infinite redirect loops). + self.assertEqual( + self.get_response(usage_key=self.sequence.location).status_code, + 200 + ) + class TestRenderXBlockSelfPaced(TestRenderXBlock): """ diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 33d13c5355..fa87183d8a 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -1612,6 +1612,43 @@ def _track_successful_certificate_generation(user_id, course_id): }) +def enclosing_sequence_for_gating_checks(block): + """ + Return the first ancestor of this block that is a SequenceDescriptor. + + Returns None if there is no such ancestor. Returns None if you call it on a + SequenceDescriptor directly. + + We explicitly test against the three known tag types that map to sequences + (even though two of them have been long since deprecated and are never + used). We _don't_ test against SequentialDescriptor directly because: + + 1. A direct comparison on the type fails because we magically mix it into a + SequenceDescriptorWithMixins object. + 2. An isinstance check doesn't give us the right behavior because Courses + and Sections both subclass SequenceDescriptor. >_< + + Also important to note that some content isn't contained in Sequences at + all. LabXchange uses learning pathways, but even content inside courses like + `static_tab`, `book`, and `about` live outside the sequence hierarchy. + """ + seq_tags = ['sequential', 'problemset', 'videosequence'] + + # If it's being called on a Sequence itself, then don't bother crawling the + # ancestor tree, because all the sequence metadata we need for gating checks + # will happen automatically when rendering the render_xblock view anyway, + # and we don't want weird, weird edge cases where you have nested Sequences + # (which would probably "work" in terms of OLX import). + if block.location.block_type in seq_tags: + return None + + ancestor = block + while ancestor and ancestor.location.block_type not in seq_tags: + ancestor = ancestor.get_parent() # Note: CourseDescriptor's parent is None + + return ancestor + + @require_http_methods(["GET", "POST"]) @ensure_valid_usage_key @xframe_options_exempt @@ -1666,6 +1703,39 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True): missed_deadlines, missed_gated_content = dates_banner_should_display(course_key, request.user) + # Some content gating happens only at the Sequence level (e.g. "has this + # timed exam started?"). + ancestor_seq = enclosing_sequence_for_gating_checks(block) + if ancestor_seq: + seq_usage_key = ancestor_seq.location + # We have a Descriptor, but I had trouble getting a SequenceModule + # from it (even using ._xmodule to force the conversion) because the + # runtime wasn't properly initialized. This view uses multiple + # runtimes (including Blockstore), so I'm pulling it from scratch + # based on the usage_key. We'll have to watch the performance impact + # of this. :( + seq_module_descriptor, _ = get_module_by_usage_id( + request, str(course_key), str(seq_usage_key), disable_staff_debug_info=True, course=course + ) + + # I'm not at all clear why get_module_by_usage_id returns the + # descriptor or why I need to manually force it to load the module + # like this manually instead of the proxying working, but trial and + # error has led me here. Hopefully all this weirdness goes away when + # SequenceModule gets converted to an XBlock in: + # https://github.com/edx/edx-platform/pull/25965 + seq_module = seq_module_descriptor._xmodule # pylint: disable=protected-access + + # If the SequenceModule feels that gating is necessary, redirect + # there so we can have some kind of error message at any rate. + if seq_module.descendants_are_gated(): + return redirect( + reverse( + 'render_xblock', + kwargs={'usage_key_string': str(seq_module.location)} + ) + ) + context = { 'fragment': block.render(requested_view, context=student_view_context), 'course': course,