diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index c20499ec22..6995f6b858 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -284,9 +284,14 @@ class ItemFactory(XModuleFactory): category = 'chapter' parent = None + descriptive_tag = None + @lazy_attribute_sequence def display_name(self, n): - return "{} {}".format(self.category, n) + if self.descriptive_tag: + return "{} {} - {}".format(self.category, n, self.descriptive_tag) + else: + return "{} {}".format(self.category, n) @lazy_attribute def location(self): @@ -355,6 +360,10 @@ class ItemFactory(XModuleFactory): user_id = kwargs.pop('user_id', ModuleStoreEnum.UserID.test) publish_item = kwargs.pop('publish_item', True) + # Remove the descriptive_tag, it's just for generating display_name, + # and doesn't need to be passed into the object constructor + kwargs.pop('descriptive_tag') + assert isinstance(location, UsageKey) assert location != parent_location diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 60391f3fb0..ae351b2ebd 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -549,10 +549,16 @@ def _has_access_descriptor(user, action, descriptor, course_key=None): return staff_access_response return ( - _visible_to_nonstaff_users(descriptor) and + _visible_to_nonstaff_users(descriptor, display_error_to_user=False) and ( _has_detached_class_tag(descriptor) or - check_start_date(user, descriptor.days_early_for_beta, descriptor.start, course_key) + check_start_date( + user, + descriptor.days_early_for_beta, + descriptor.start, + course_key, + display_error_to_user=False + ) ) ) @@ -782,14 +788,19 @@ def _has_staff_access_to_descriptor(user, descriptor, course_key): return _has_staff_access_to_location(user, descriptor.location, course_key) -def _visible_to_nonstaff_users(descriptor): +def _visible_to_nonstaff_users(descriptor, display_error_to_user=True): """ Returns if the object is visible to nonstaff users. Arguments: descriptor: object to check + display_error_to_user: If True, show an error message to the user say the content was hidden. Otherwise, + hide the content silently. """ - return VisibilityError() if descriptor.visible_to_staff_only else ACCESS_GRANTED + if descriptor.visible_to_staff_only: + return VisibilityError(display_error_to_user=display_error_to_user) + else: + return ACCESS_GRANTED def _can_access_descriptor_with_milestones(user, descriptor, course_key): diff --git a/lms/djangoapps/courseware/access_response.py b/lms/djangoapps/courseware/access_response.py index 3dd810998c..2bc02c209b 100644 --- a/lms/djangoapps/courseware/access_response.py +++ b/lms/djangoapps/courseware/access_response.py @@ -121,7 +121,11 @@ class StartDateError(AccessError): Access denied because the course has not started yet and the user is not staff """ - def __init__(self, start_date): + def __init__(self, start_date, display_error_to_user=True): + """ + Arguments: + display_error_to_user: If True, display this error to users in the UI. + """ error_code = "course_not_started" if start_date == DEFAULT_START_DATE: developer_message = u"Course has not started" @@ -130,7 +134,11 @@ class StartDateError(AccessError): developer_message = u"Course does not start until {}".format(start_date) user_message = _(u"Course does not start until {}" .format(u"{:%B %d, %Y}".format(start_date))) - super(StartDateError, self).__init__(error_code, developer_message, user_message) + super(StartDateError, self).__init__( + error_code, + developer_message, + user_message if display_error_to_user else None + ) class MilestoneAccessError(AccessError): @@ -149,11 +157,20 @@ class VisibilityError(AccessError): Access denied because the user does have the correct role to view this course. """ - def __init__(self): + def __init__(self, display_error_to_user=True): + """ + Arguments: + display_error_to_user: Should a message showing that access was denied to this content + be shown to the user? + """ error_code = "not_visible_to_user" developer_message = u"Course is not visible to this user" user_message = _(u"You do not have access to this course") - super(VisibilityError, self).__init__(error_code, developer_message, user_message) + super(VisibilityError, self).__init__( + error_code, + developer_message, + user_message if display_error_to_user else None + ) class MobileAvailabilityError(AccessError): diff --git a/lms/djangoapps/courseware/access_utils.py b/lms/djangoapps/courseware/access_utils.py index e76b66e959..58f4afa8dd 100644 --- a/lms/djangoapps/courseware/access_utils.py +++ b/lms/djangoapps/courseware/access_utils.py @@ -58,11 +58,14 @@ def adjust_start_date(user, days_early_for_beta, start, course_key): return start -def check_start_date(user, days_early_for_beta, start, course_key): +def check_start_date(user, days_early_for_beta, start, course_key, display_error_to_user=True): """ Verifies whether the given user is allowed access given the start date and the Beta offset for the given course. + Arguments: + display_error_to_user: If True, display this error to users in the UI. + Returns: AccessResponse: Either ACCESS_GRANTED or StartDateError. """ @@ -80,7 +83,7 @@ def check_start_date(user, days_early_for_beta, start, course_key): if now > effective_start: return ACCESS_GRANTED - return StartDateError(start) + return StartDateError(start, display_error_to_user=display_error_to_user) def in_preview_mode(): diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index fe5383430d..5f41c83d3b 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -290,7 +290,7 @@ def _add_timed_exam_info(user, course, section, section_context): def get_module(user, request, usage_key, field_data_cache, position=None, log_if_not_found=True, wrap_xmodule_display=True, grade_bucket_type=None, depth=0, - static_asset_path='', course=None): + static_asset_path='', course=None, will_recheck_access=False): """ Get an instance of the xmodule class identified by location, setting the state based on an existing StudentModule, or creating one if none @@ -313,6 +313,9 @@ def get_module(user, request, usage_key, field_data_cache, by get_course_info_section, because info section modules do not have a course as the parent module, and thus do not inherit this lms key value. + - will_recheck_access : If True, the caller commits to re-checking access on each child XBlock + before rendering the content in order to display access error messages + to the user. Returns: xmodule instance, or None if the user does not have access to the module. If there's an error, will try to return an instance of ErrorModule @@ -325,7 +328,7 @@ def get_module(user, request, usage_key, field_data_cache, wrap_xmodule_display=wrap_xmodule_display, grade_bucket_type=grade_bucket_type, static_asset_path=static_asset_path, - course=course) + course=course, will_recheck_access=will_recheck_access) except ItemNotFoundError: if log_if_not_found: log.debug("Error in get_module: ItemNotFoundError") @@ -392,7 +395,7 @@ def get_xqueue_callback_url_prefix(request): def get_module_for_descriptor(user, request, descriptor, field_data_cache, course_key, position=None, wrap_xmodule_display=True, grade_bucket_type=None, static_asset_path='', disable_staff_debug_info=False, - course=None): + course=None, will_recheck_access=False): """ Implements get_module, extracting out the request-specific functionality. @@ -424,7 +427,8 @@ def get_module_for_descriptor(user, request, descriptor, field_data_cache, cours user_location=user_location, request_token=xblock_request_token(request), disable_staff_debug_info=disable_staff_debug_info, - course=course + course=course, + will_recheck_access=will_recheck_access, ) @@ -443,7 +447,8 @@ def get_module_system_for_user( static_asset_path='', user_location=None, disable_staff_debug_info=False, - course=None + course=None, + will_recheck_access=False, ): """ Helper function that returns a module system and student_data bound to a user and a descriptor. @@ -514,7 +519,7 @@ def get_module_system_for_user( user_location=user_location, request_token=request_token, course=course, - will_recheck_access=True, + will_recheck_access=will_recheck_access, ) def get_event_handler(event_type): @@ -651,7 +656,8 @@ def get_module_system_for_user( static_asset_path=static_asset_path, user_location=user_location, request_token=request_token, - course=course + course=course, + will_recheck_access=will_recheck_access, ) module.descriptor.bind_for_student( @@ -872,7 +878,8 @@ def get_module_for_descriptor_internal(user, descriptor, student_data, course_id user_location=user_location, request_token=request_token, disable_staff_debug_info=disable_staff_debug_info, - course=course + course=course, + will_recheck_access=will_recheck_access, ) descriptor.bind_for_student( @@ -900,7 +907,6 @@ def get_module_for_descriptor_internal(user, descriptor, student_data, course_id not access and will_recheck_access and (access.user_message or access.user_fragment) - and isinstance(access, IncorrectPartitionGroupError) ) if access or caller_will_handle_access_error: return descriptor @@ -908,7 +914,7 @@ def get_module_for_descriptor_internal(user, descriptor, student_data, course_id return descriptor -def load_single_xblock(request, user_id, course_id, usage_key_string, course=None): +def load_single_xblock(request, user_id, course_id, usage_key_string, course=None, will_recheck_access=False): """ Load a single XBlock identified by usage_key_string. """ @@ -922,7 +928,15 @@ def load_single_xblock(request, user_id, course_id, usage_key_string, course=Non modulestore().get_item(usage_key), depth=0, ) - instance = get_module(user, request, usage_key, field_data_cache, grade_bucket_type='xqueue', course=course) + instance = get_module( + user, + request, + usage_key, + field_data_cache, + grade_bucket_type='xqueue', + course=course, + will_recheck_access=will_recheck_access + ) if instance is None: msg = u"No module {0} for user {1}--access denied?".format(usage_key_string, user) log.debug(msg) diff --git a/lms/djangoapps/courseware/views/index.py b/lms/djangoapps/courseware/views/index.py index dd059bea7a..d40cf593cc 100644 --- a/lms/djangoapps/courseware/views/index.py +++ b/lms/djangoapps/courseware/views/index.py @@ -362,6 +362,7 @@ class CoursewareIndex(View): self.field_data_cache, self.course_key, course=self.course, + will_recheck_access=True, ) def _prefetch_and_bind_section(self): @@ -382,6 +383,7 @@ class CoursewareIndex(View): self.course_key, self.position, course=self.course, + will_recheck_access=True, ) def _save_positions(self): diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index fccf725582..e0df65c83c 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -322,7 +322,13 @@ def course_info(request, course_id): course.id, request.user, course, depth=2 ) course_module = get_module_for_descriptor( - user, request, course, field_data_cache, course.id, course=course + user, + request, + course, + field_data_cache, + course.id, + course=course, + will_recheck_access=True, ) chapter_module = get_current_child(course_module) if chapter_module is not None: diff --git a/lms/templates/seq_module.html b/lms/templates/seq_module.html index de1df5628a..6ec3acdc86 100644 --- a/lms/templates/seq_module.html +++ b/lms/templates/seq_module.html @@ -54,8 +54,8 @@ id="tab_${idx}"> % if 'complete' in item: - diff --git a/openedx/features/content_type_gating/tests/test_access.py b/openedx/features/content_type_gating/tests/test_access.py index 65413d2907..2d8d2d7ad0 100644 --- a/openedx/features/content_type_gating/tests/test_access.py +++ b/openedx/features/content_type_gating/tests/test_access.py @@ -4,16 +4,21 @@ Test audit user's access to various content based on content-gating features. from __future__ import absolute_import import json +import os from datetime import datetime, timedelta import ddt import six from django.conf import settings -from django.test.client import RequestFactory +from django.test.client import RequestFactory, Client from django.test.utils import override_settings from django.urls import reverse from django.utils import timezone +from django.contrib.auth.models import User from mock import patch +from pyquery import PyQuery as pq + +from six.moves.html_parser import HTMLParser from course_api.blocks.api import get_blocks from course_modes.tests.factories import CourseModeFactory @@ -57,9 +62,9 @@ METADATA = { @patch("crum.get_current_request") -def _get_fragment_from_block(block, user_id, course, request_factory, mock_get_current_request): +def _get_content_from_fragment(block, user_id, course, request_factory, mock_get_current_request): """ - Returns the rendered fragment of a block + Returns the content from the rendered fragment of a block Arguments: block: some sort of xblock descriptor, must implement .scope_ids.usage_id user_id (int): id of user @@ -69,20 +74,39 @@ def _get_fragment_from_block(block, user_id, course, request_factory, mock_get_c mock_get_current_request.return_value = fake_request # Load a block we know will pass access control checks - vertical_xblock = load_single_xblock( + block = load_single_xblock( request=fake_request, user_id=user_id, course_id=six.text_type(course.id), - usage_key_string=six.text_type(course.scope_ids.usage_id), - course=course + usage_key_string=six.text_type(block.scope_ids.usage_id), + course=course, + will_recheck_access=True, ) - runtime = vertical_xblock.runtime - - # This method of fetching the block from the descriptor bypassess access checks - problem_block = runtime.get_module(block) # Attempt to render the block, this should return different fragments if the content is gated or not. - frag = runtime.render(problem_block, 'student_view') - return frag + frag = block.render('student_view') + return frag.content + + +def _get_content_from_lms_index(block, user_id, course, request_factory): + """ + Returns the content from the lms index view of the block + Arguments: + block: some sort of xblock descriptor, must implement .scope_ids.usage_id + user_id (int): id of user + course_id (CourseLocator): id of course + """ + client = Client() + client.login(username=User.objects.get(id=user_id).username, password=TEST_PASSWORD) + page_content = client.get( + reverse('courseware', kwargs={'course_id': block.scope_ids.usage_id.course_key}), + follow=True, + ) + page = pq(page_content.content) + seq_contents = page('#seq_contents_0').html() + seq = pq(seq_contents) + block_contents = seq('[data-id="{}"]'.format(block.scope_ids.usage_id)) + + return block_contents.html() def _assert_block_is_gated(block, is_gated, user, course, request_factory, has_upgrade_link=True): @@ -95,16 +119,17 @@ def _assert_block_is_gated(block, is_gated, user, course, request_factory, has_u course_id (CourseLocator): id of course """ checkout_link = '#' if has_upgrade_link else None - with patch.object(ContentTypeGatingPartition, '_get_checkout_link', return_value=checkout_link): - frag = _get_fragment_from_block(block, user.id, course, request_factory) - if is_gated: - assert 'content-paywall' in frag.content - if has_upgrade_link: - assert 'certA_1' in frag.content + for content_getter in (_get_content_from_fragment, _get_content_from_lms_index): + with patch.object(ContentTypeGatingPartition, '_get_checkout_link', return_value=checkout_link): + content = content_getter(block, user.id, course, request_factory) + if is_gated: + assert 'content-paywall' in content + if has_upgrade_link: + assert 'certA_1' in content + else: + assert 'certA_1' not in content else: - assert 'certA_1' not in frag.content - else: - assert 'content-paywall' not in frag.content + assert 'content-paywall' not in content fake_request = request_factory.get('') with patch('lms.djangoapps.course_api.blocks.api.is_request_from_mobile_app', return_value=True): @@ -125,8 +150,8 @@ def _assert_block_is_empty(block, user_id, course, request_factory): user_id (int): id of user course_id (CourseLocator): id of course """ - frag = _get_fragment_from_block(block, user_id, course, request_factory) - assert frag.content == u'' + content = _get_content_from_lms_index(block, user_id, course, request_factory) + assert content is None @ddt.ddt @@ -183,48 +208,88 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase): # Therefore, we create a problem component when has_score is True # and an html component when has_score is False. category='problem' if has_score else 'html', - display_name=case_name, graded=graded, weight=weight, metadata=METADATA if (graded and has_score and weight) else {}, ) + # Intersperse HTML so that the content-gating renders in all blocks + ItemFactory.create( + parent=cls.blocks_dict['vertical'], + category='html', + graded=False, + ) cls.graded_score_weight_blocks[(graded, has_score, weight)] = block + host = os.environ.get('BOK_CHOY_HOSTNAME', '127.0.0.1') + metadata_lti_xblock = { + 'lti_id': 'correct_lti_id', + 'launch_url': 'http://{}:{}/{}'.format(host, '8765', 'correct_lti_endpoint'), + 'open_in_a_new_page': False + } + + scored_lti_metadata = {} + scored_lti_metadata.update(metadata_lti_xblock) + scored_lti_metadata.update(METADATA) + # add LTI blocks to default course cls.blocks_dict['lti_block'] = ItemFactory.create( parent=cls.blocks_dict['vertical'], category='lti_consumer', - display_name='lti_consumer', has_score=True, graded=True, - metadata=METADATA, + metadata=scored_lti_metadata, + ) + # Intersperse HTML so that the content-gating renders in all blocks + ItemFactory.create( + parent=cls.blocks_dict['vertical'], + category='html', + graded=False, ) cls.blocks_dict['lti_block_not_scored'] = ItemFactory.create( parent=cls.blocks_dict['vertical'], category='lti_consumer', - display_name='lti_consumer_2', has_score=False, + metadata=metadata_lti_xblock, + ) + + # Intersperse HTML so that the content-gating renders in all blocks + ItemFactory.create( + parent=cls.blocks_dict['vertical'], + category='html', + graded=False, ) # add ungraded problem for xblock_handler test cls.blocks_dict['graded_problem'] = ItemFactory.create( parent=cls.blocks_dict['vertical'], category='problem', - display_name='graded_problem', graded=True, metadata=METADATA, ) + + # Intersperse HTML so that the content-gating renders in all blocks + ItemFactory.create( + parent=cls.blocks_dict['vertical'], + category='html', + graded=False, + ) + cls.blocks_dict['ungraded_problem'] = ItemFactory.create( parent=cls.blocks_dict['vertical'], category='problem', - display_name='ungraded_problem', + graded=False, + ) + + # Intersperse HTML so that the content-gating renders in all blocks + ItemFactory.create( + parent=cls.blocks_dict['vertical'], + category='html', graded=False, ) cls.blocks_dict['audit_visible_graded_problem'] = ItemFactory.create( parent=cls.blocks_dict['vertical'], category='problem', - display_name='audit_visible_graded_problem', graded=True, group_access={ CONTENT_GATING_PARTITION_ID: [ @@ -234,6 +299,13 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase): }, ) + # Intersperse HTML so that the content-gating renders in all blocks + ItemFactory.create( + parent=cls.blocks_dict['vertical'], + category='html', + graded=False, + ) + # audit_only course only has an audit track available cls.courses['audit_only'] = cls._create_course( run='audit_only_course_run_1', @@ -350,11 +422,16 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase): block = ItemFactory.create( parent=blocks_dict['vertical'], category=component_type, - display_name=component_type, graded=True, metadata={} if (component_type == 'html' or len(modes) == 1) else METADATA ) blocks_dict[component_type] = block + # Intersperse HTML so that the content-gating renders in all blocks + ItemFactory.create( + parent=blocks_dict['vertical'], + category='html', + graded=False, + ) return { 'course': course, @@ -467,7 +544,7 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase): (404) and when it is available to audit users (200). Content is always available to verified users. """ - problem_location = self.course.id.make_usage_key(xblock_type, xblock_name) + problem_location = self.blocks_dict[xblock_name].scope_ids.usage_id url = reverse( 'xblock_handler', kwargs={ @@ -500,6 +577,13 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase): user = role_factory.create() else: user = role_factory.create(course_key=self.course.id) + + CourseEnrollmentFactory.create( + user=user, + course_id=self.course.id, + mode='audit', + ) + # assert that course team members have access to graded content _assert_block_is_gated( block=self.blocks_dict['problem'], @@ -523,6 +607,11 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase): role = RoleFactory(name=role_name, course_id=self.course.id) role.users.add(user) + CourseEnrollmentFactory.create( + user=user, + course_id=self.course.id, + mode='audit', + ) _assert_block_is_gated( block=self.blocks_dict['problem'], user=user, @@ -674,6 +763,13 @@ class TestProblemTypeAccess(SharedModuleStoreTestCase): user = UserFactory.create() role = RoleFactory(name=role_name, course_id=self.course.id) role.users.add(user) + + CourseEnrollmentFactory.create( + user=user, + course_id=self.course.id, + mode='audit' + ) + self.update_masquerade(username=user.username) _assert_block_is_gated( @@ -733,13 +829,11 @@ class TestConditionalContentAccess(TestConditionalContent): self.block_a = ItemFactory.create( category='problem', parent=self.vertical_a, - display_name='problem_a', metadata=METADATA, ) self.block_b = ItemFactory.create( category='problem', parent=self.vertical_b, - display_name='problem_b', metadata=METADATA, ) @@ -843,7 +937,6 @@ class TestMessageDeduplication(ModuleStoreTestCase): blocks_dict['graded_1'] = ItemFactory.create( parent=blocks_dict['vertical'], category='problem', - display_name='graded_problem', graded=True, metadata=METADATA, ) @@ -862,14 +955,12 @@ class TestMessageDeduplication(ModuleStoreTestCase): blocks_dict['graded_1'] = ItemFactory.create( parent=blocks_dict['vertical'], category='problem', - display_name='graded_problem', graded=True, metadata=METADATA, ) blocks_dict['graded_2'] = ItemFactory.create( parent=blocks_dict['vertical'], category='problem', - display_name='graded_problem', graded=True, metadata=METADATA, ) @@ -899,28 +990,24 @@ class TestMessageDeduplication(ModuleStoreTestCase): blocks_dict['graded_1'] = ItemFactory.create( parent=blocks_dict['vertical'], category='problem', - display_name='graded_problem', graded=True, metadata=METADATA, ) blocks_dict['graded_2'] = ItemFactory.create( parent=blocks_dict['vertical'], category='problem', - display_name='graded_problem', graded=True, metadata=METADATA, ) blocks_dict['graded_3'] = ItemFactory.create( parent=blocks_dict['vertical'], category='problem', - display_name='graded_problem', graded=True, metadata=METADATA, ) blocks_dict['graded_4'] = ItemFactory.create( parent=blocks_dict['vertical'], category='problem', - display_name='graded_problem', graded=True, metadata=METADATA, ) @@ -962,20 +1049,17 @@ class TestMessageDeduplication(ModuleStoreTestCase): blocks_dict['graded_1'] = ItemFactory.create( parent=blocks_dict['vertical'], category='problem', - display_name='graded_problem', graded=True, metadata=METADATA, ) blocks_dict['ungraded_2'] = ItemFactory.create( parent=blocks_dict['vertical'], category='problem', - display_name='ungraded_problem', graded=False, ) blocks_dict['graded_3'] = ItemFactory.create( parent=blocks_dict['vertical'], category='problem', - display_name='graded_problem', graded=True, metadata=METADATA, )