diff --git a/common/lib/xmodule/xmodule/partitions/partitions.py b/common/lib/xmodule/xmodule/partitions/partitions.py index 9c62c957ac..49aa36e2ef 100644 --- a/common/lib/xmodule/xmodule/partitions/partitions.py +++ b/common/lib/xmodule/xmodule/partitions/partitions.py @@ -250,13 +250,13 @@ class UserPartition(namedtuple("UserPartition", "id name description groups sche ) ) - def access_denied_message(self, block_key, user, user_group, allowed_groups): + def access_denied_message(self, block, user, user_group, allowed_groups): """ Return a message that should be displayed to the user when they are not allowed to access content managed by this partition, or None if there is no applicable message. Arguments: - block_key (:class:`.BlockUsageLocator`): The content being managed + block (:class:`.XBlock`): The content being managed user (:class:`.User`): The user who was denied access user_group (:class:`.Group`): The current Group the user is in allowed_groups (list of :class:`.Group`): The groups who are allowed to see the content diff --git a/common/lib/xmodule/xmodule/partitions/partitions_service.py b/common/lib/xmodule/xmodule/partitions/partitions_service.py index 3fb0d8740d..e742373288 100644 --- a/common/lib/xmodule/xmodule/partitions/partitions_service.py +++ b/common/lib/xmodule/xmodule/partitions/partitions_service.py @@ -104,7 +104,7 @@ def _create_enrollment_track_partition(course): log.warning( "Can't add 'enrollment_track' partition, as ID {id} is assigned to {partition} in course {course}.".format( id=ENROLLMENT_TRACK_PARTITION_ID, - partition=get_partition_from_id(course.user_partitions, ENROLLMENT_TRACK_PARTITION_ID).name, + partition=_get_partition_from_id(course.user_partitions, ENROLLMENT_TRACK_PARTITION_ID).name, course=unicode(course.id) ) ) @@ -193,7 +193,7 @@ class PartitionService(object): Returns: A UserPartition, or None if not found. """ - return get_partition_from_id(self.course_partitions, user_partition_id) + return _get_partition_from_id(self.course_partitions, user_partition_id) def get_group(self, user, user_partition, assign=True): """ @@ -206,7 +206,7 @@ class PartitionService(object): ) -def get_partition_from_id(partitions, user_partition_id): +def _get_partition_from_id(partitions, user_partition_id): """ Look for a user partition with a matching id in the provided list of partitions. diff --git a/lms/djangoapps/course_api/blocks/api.py b/lms/djangoapps/course_api/blocks/api.py index a99fc4121d..8c0c586db4 100644 --- a/lms/djangoapps/course_api/blocks/api.py +++ b/lms/djangoapps/course_api/blocks/api.py @@ -5,7 +5,6 @@ API function for retrieving course blocks data import lms.djangoapps.course_blocks.api as course_blocks_api from lms.djangoapps.course_blocks.transformers.hidden_content import HiddenContentTransformer from lms.djangoapps.course_blocks.transformers.hide_empty import HideEmptyTransformer -from lms.djangoapps.course_blocks.transformers.access_denied_filter import AccessDeniedMessageFilterTransformer from openedx.core.djangoapps.content.block_structure.transformers import BlockStructureTransformers from openedx.core.lib.mobile_utils import is_request_from_mobile_app @@ -26,7 +25,6 @@ def get_blocks( student_view_data=None, return_type='dict', block_types_filter=None, - hide_access_denials=False, ): """ Return a serialized representation of the course blocks. @@ -53,9 +51,6 @@ def get_blocks( the format for returning the blocks. block_types_filter (list): Optional list of block type names used to filter the final result of returned blocks. - hide_access_denials (bool): When True, filter out any blocks that were - denied access to the user, even if they have access denial messages - attached. """ # create ordered list of transformers, adding BlocksAPITransformer at end. transformers = BlockStructureTransformers() @@ -75,9 +70,6 @@ def get_blocks( HiddenContentTransformer() ] - if hide_access_denials: - transformers += [AccessDeniedMessageFilterTransformer()] - # TODO: Remove this after REVE-52 lands and old-mobile-app traffic falls to < 5% of mobile traffic if is_request_from_mobile_app(request): transformers += [HideEmptyTransformer()] diff --git a/lms/djangoapps/course_api/blocks/serializers.py b/lms/djangoapps/course_api/blocks/serializers.py index 3bbd41aa35..f154b2234d 100644 --- a/lms/djangoapps/course_api/blocks/serializers.py +++ b/lms/djangoapps/course_api/blocks/serializers.py @@ -35,20 +35,6 @@ class BlockSerializer(serializers.Serializer): # pylint: disable=abstract-metho Return a serializable representation of the requested block """ # create response data dict for basic fields - - block_structure = self.context['block_structure'] - authorization_denial_reason = block_structure.get_xblock_field(block_key, 'authorization_denial_reason') - authorization_denial_message = block_structure.get_xblock_field(block_key, 'authorization_denial_message') - - if authorization_denial_reason and authorization_denial_message: - data = { - 'id': unicode(block_key), - 'block_id': unicode(block_key.block_id), - 'authorization_denial_reason': authorization_denial_reason, - 'authorization_denial_message': authorization_denial_message - } - return data - data = { 'id': unicode(block_key), 'block_id': unicode(block_key.block_id), @@ -85,7 +71,7 @@ class BlockSerializer(serializers.Serializer): # pylint: disable=abstract-metho data[supported_field.serializer_field_name] = field_value if 'children' in self.context['requested_fields']: - children = block_structure.get_children(block_key) + children = self.context['block_structure'].get_children(block_key) if children: data['children'] = [unicode(child) for child in children] diff --git a/lms/djangoapps/course_api/blocks/urls.py b/lms/djangoapps/course_api/blocks/urls.py index ea71b1581e..7fb3ca83f9 100644 --- a/lms/djangoapps/course_api/blocks/urls.py +++ b/lms/djangoapps/course_api/blocks/urls.py @@ -11,7 +11,6 @@ urlpatterns = [ url( r'^v1/blocks/{}'.format(settings.USAGE_KEY_PATTERN), BlocksView.as_view(), - kwargs={'hide_access_denials': True}, name="blocks_in_block_tree" ), @@ -19,20 +18,6 @@ urlpatterns = [ url( r'^v1/blocks/', BlocksInCourseView.as_view(), - kwargs={'hide_access_denials': True}, - name="blocks_in_course" - ), - # This endpoint requires the usage_key for the starting block. - url( - r'^v2/blocks/{}'.format(settings.USAGE_KEY_PATTERN), - BlocksView.as_view(), - name="blocks_in_block_tree" - ), - - # This endpoint is an alternative to the above, but requires course_id as a parameter. - url( - r'^v2/blocks/', - BlocksInCourseView.as_view(), name="blocks_in_course" ), ] diff --git a/lms/djangoapps/course_api/blocks/views.py b/lms/djangoapps/course_api/blocks/views.py index 8f40a223df..02783f3b78 100644 --- a/lms/djangoapps/course_api/blocks/views.py +++ b/lms/djangoapps/course_api/blocks/views.py @@ -183,7 +183,7 @@ class BlocksView(DeveloperErrorViewMixin, ListAPIView): Returned only if "show_correctness" is included in the "requested_fields" parameter. """ - def list(self, request, usage_key_string, hide_access_denials=False): # pylint: disable=arguments-differ + def list(self, request, usage_key_string): # pylint: disable=arguments-differ """ REST API endpoint for listing all the blocks information in the course, while regarding user access and roles. @@ -213,7 +213,6 @@ class BlocksView(DeveloperErrorViewMixin, ListAPIView): params.cleaned_data.get('student_view_data', []), params.cleaned_data['return_type'], params.cleaned_data.get('block_types_filter', None), - hide_access_denials=hide_access_denials, ) ) except ItemNotFoundError as exception: @@ -261,7 +260,7 @@ class BlocksInCourseView(BlocksView): with a message indicating that the course_id is not valid. """ - def list(self, request, hide_access_denials=False): # pylint: disable=arguments-differ + def list(self, request): # pylint: disable=arguments-differ """ Retrieves the usage_key for the requested course, and then returns the same information that would be returned by BlocksView.list, called with @@ -281,4 +280,4 @@ class BlocksInCourseView(BlocksView): course_usage_key = modulestore().make_course_usage_key(course_key) except InvalidKeyError: raise ValidationError(u"'{}' is not a valid course key.".format(unicode(course_key_string))) - return super(BlocksInCourseView, self).list(request, course_usage_key, hide_access_denials=hide_access_denials) + return super(BlocksInCourseView, self).list(request, course_usage_key) diff --git a/lms/djangoapps/course_api/tests/test_serializers.py b/lms/djangoapps/course_api/tests/test_serializers.py index 45197a6422..59d073b93a 100644 --- a/lms/djangoapps/course_api/tests/test_serializers.py +++ b/lms/djangoapps/course_api/tests/test_serializers.py @@ -65,7 +65,7 @@ class TestCourseSerializer(CourseApiFactoryMixin, ModuleStoreTestCase): 'end': u'2015-09-19T18:00:00Z', 'enrollment_start': u'2015-06-15T00:00:00Z', 'enrollment_end': u'2015-07-15T00:00:00Z', - 'blocks_url': u'http://testserver/api/courses/v2/blocks/?course_id=edX%2Ftoy%2F2012_Fall', + 'blocks_url': u'http://testserver/api/courses/v1/blocks/?course_id=edX%2Ftoy%2F2012_Fall', 'effort': u'6 hours', 'pacing': 'instructor', 'mobile_available': True, diff --git a/lms/djangoapps/course_blocks/transformers/access_denied_filter.py b/lms/djangoapps/course_blocks/transformers/access_denied_filter.py deleted file mode 100644 index 5ca3125857..0000000000 --- a/lms/djangoapps/course_blocks/transformers/access_denied_filter.py +++ /dev/null @@ -1,43 +0,0 @@ -""" -Access Denied Message Filter Transformer implementation. -""" -# TODO: Remove this file after REVE-52 lands and old-mobile-app traffic falls to < 5% of mobile traffic -from openedx.core.djangoapps.content.block_structure.transformer import ( - BlockStructureTransformer -) - - -class AccessDeniedMessageFilterTransformer(BlockStructureTransformer): - """ - A transformer that removes any block from the course that has an - authorization_denial_reason or an authorization_denial_message. - """ - WRITE_VERSION = 1 - READ_VERSION = 1 - - @classmethod - def name(cls): - """ - Unique identifier for the transformer's class; - same identifier used in setup.py. - """ - return "access_denied_message_filter" - - @classmethod - def collect(cls, block_structure): - """ - Collects any information that's necessary to execute this - transformer's transform method. - """ - block_structure.request_xblock_fields('authorization_denial_reason', 'authorization_denial_message') - - def transform(self, usage_info, block_structure): - def _filter(block_key): - reason = block_structure.get_xblock_field(block_key, 'authorization_denial_reason') - message = block_structure.get_xblock_field(block_key, 'authorization_denial_message') - return reason and message - - for _ in block_structure.post_order_traversal( - filter_func=block_structure.create_removal_filter(_filter) - ): - pass diff --git a/lms/djangoapps/course_blocks/transformers/tests/test_split_test.py b/lms/djangoapps/course_blocks/transformers/tests/test_split_test.py index 9cf0f8b071..1e8873e0f9 100644 --- a/lms/djangoapps/course_blocks/transformers/tests/test_split_test.py +++ b/lms/djangoapps/course_blocks/transformers/tests/test_split_test.py @@ -177,7 +177,7 @@ class SplitTestTransformerTestCase(CourseStructureTestCase): # parents. However, we don't think this is a use case we need to # support for split_test components (since they are now deprecated # in favor of content groups and user partitions). - (0, ('course', 'A', 'D', 'E', 'H', 'L', 'O', 'P', )), + (0, ('course', 'A', 'D', 'E', 'H', 'L', 'O', 'P',)), (1, ('course', 'A', 'D', 'F', 'J', 'M', 'I',)), (2, ('course', 'A', 'D', 'G', 'O',)), ) diff --git a/lms/djangoapps/course_blocks/transformers/tests/test_user_partitions.py b/lms/djangoapps/course_blocks/transformers/tests/test_user_partitions.py index d4ad5d6e0b..67a8ded484 100644 --- a/lms/djangoapps/course_blocks/transformers/tests/test_user_partitions.py +++ b/lms/djangoapps/course_blocks/transformers/tests/test_user_partitions.py @@ -4,18 +4,13 @@ Tests for UserPartitionTransformer. """ import string from collections import namedtuple -from datetime import datetime import ddt -from mock import patch -from course_modes.tests.factories import CourseModeFactory from openedx.core.djangoapps.course_groups.cohorts import add_user_to_cohort from openedx.core.djangoapps.course_groups.partition_scheme import CohortPartitionScheme from openedx.core.djangoapps.course_groups.tests.helpers import CohortFactory, config_course_cohorts from openedx.core.djangoapps.course_groups.views import link_cohort_to_partition_group -from openedx.features.content_type_gating.models import ContentTypeGatingConfig -from openedx.features.content_type_gating.partitions import create_content_gating_partition from student.tests.factories import CourseEnrollmentFactory from xmodule.modulestore.tests.factories import CourseFactory from xmodule.partitions.partitions import Group, UserPartition @@ -178,7 +173,7 @@ class UserPartitionTransformerTestCase(UserPartitionTestMixin, CourseStructureTe '#type': 'vertical', '#ref': 'K', '#parents': ['E'], - 'metadata': {'group_access': {self.user_partition.id: [4, 51]}}, + 'metadata': {'group_access': {self.user_partition.id: [4]}}, '#children': [{'#type': 'vertical', '#ref': 'N'}], }, { @@ -224,32 +219,6 @@ class UserPartitionTransformerTestCase(UserPartitionTestMixin, CourseStructureTe self.get_block_key_set(self.blocks, *expected_blocks) ) - def test_transform_with_content_gating_partition(self): - self.setup_partitions_and_course() - CourseModeFactory.create(course_id=self.course.id, mode_slug='audit') - CourseModeFactory.create(course_id=self.course.id, mode_slug='verified') - ContentTypeGatingConfig.objects.create(enabled=True, enabled_as_of=datetime(2018, 1, 1)) - partition = create_content_gating_partition(self.course) - self.user_partitions.append(partition) - cohort = self.partition_cohorts[0][1] - add_user_to_cohort(cohort, self.user.username) - - with patch( - 'lms.djangoapps.course_blocks.transformers.user_partitions.get_partition_from_id', - return_value=partition - ), patch( - 'lms.djangoapps.course_blocks.transformers.user_partitions._MergedGroupAccess.get_allowed_groups', - return_value={51: set([])} - ): - trans_block_structure = get_course_blocks( - self.user, - self.course.location, - self.transformers, - ) - xblocks_denial_reason = [trans_block_structure.get_xblock_field(b, 'authorization_denial_reason') - for b in trans_block_structure.get_block_keys()] - self.assertSetEqual(set(xblocks_denial_reason), set([u'Feature-based Enrollments'])) - def test_transform_on_inactive_partition(self): """ Tests UserPartitionTransformer for inactive UserPartition. diff --git a/lms/djangoapps/course_blocks/transformers/user_partitions.py b/lms/djangoapps/course_blocks/transformers/user_partitions.py index 19519b01f2..4efd932b2a 100644 --- a/lms/djangoapps/course_blocks/transformers/user_partitions.py +++ b/lms/djangoapps/course_blocks/transformers/user_partitions.py @@ -6,11 +6,7 @@ from openedx.core.djangoapps.content.block_structure.transformer import ( BlockStructureTransformer, FilteringTransformerMixin ) -from xmodule.partitions.partitions_service import ( - get_all_partitions_for_course, - get_partition_from_id, - get_user_partition_groups -) +from xmodule.partitions.partitions_service import get_user_partition_groups, get_all_partitions_for_course from .split_test import SplitTestTransformer from .utils import get_field_on_block @@ -83,38 +79,15 @@ class UserPartitionTransformer(FilteringTransformerMixin, BlockStructureTransfor return [block_structure.create_universal_filter()] user_groups = get_user_partition_groups(usage_info.course_key, user_partitions, user, 'id') - - for block_key in block_structure.topological_traversal(): - transformer_block_field = block_structure.get_transformer_block_field( - block_key, self, 'merged_group_access' - ) - access_denying_partition_id = transformer_block_field.get_access_denying_partition( - user_groups - ) - access_denying_partition = get_partition_from_id(user_partitions, access_denying_partition_id) - - if not has_access(user, 'staff', block_key) and access_denying_partition: - user_group = user_groups.get(access_denying_partition.id) - allowed_groups = transformer_block_field.get_allowed_groups()[access_denying_partition.id] - access_denied_message = access_denying_partition.access_denied_message( - block_key, user, user_group, allowed_groups - ) - block_structure.override_xblock_field( - block_key, 'authorization_denial_reason', access_denying_partition.name - ) - block_structure.override_xblock_field( - block_key, 'authorization_denial_message', access_denied_message - ) - group_access_filter = block_structure.create_removal_filter( - lambda block_key: ( - not has_access(user, 'staff', block_key) and - block_structure.get_transformer_block_field( - block_key, self, 'merged_group_access' - ).get_access_denying_partition(user_groups) is not None and - block_structure.get_xblock_field(block_key, 'authorization_denial_message') is None + lambda block_key: not ( + has_access(user, 'staff', block_key) or + block_structure.get_transformer_block_field(block_key, self, 'merged_group_access').check_group_access( + user_groups + ) ) ) + result_list.append(group_access_filter) return result_list @@ -180,6 +153,7 @@ class _MergedGroupAccess(object): # Set the default to universal access, for the case when # there are no parents. merged_parent_group_ids = None + if merged_parent_access_list: # Set the default to most restrictive as we iterate # through all the parent chains. @@ -214,9 +188,6 @@ class _MergedGroupAccess(object): if merged_group_ids is not None: self._access[partition.id] = merged_group_ids - def get_allowed_groups(self): - return self._access - @staticmethod def _intersection(*sets): """ @@ -239,34 +210,6 @@ class _MergedGroupAccess(object): else: return None - def get_access_denying_partition(self, user_groups): - """ - Arguments: - dict[int: Group]: Given a user, a mapping from user - partition IDs to the group to which the user belongs in - each partition. - - Returns: - bool: Which partition is denying access - """ - for partition_id, allowed_group_ids in self._access.iteritems(): - # If the user is not assigned to a group for this partition, - # return partition that would deny access. - if partition_id not in user_groups: - return partition_id - - # If the user belongs to one of the allowed groups for this - # partition, then move and check the next partition. - elif user_groups[partition_id].id in allowed_group_ids: - continue - - # Else, return partition that would deny access. - else: - return partition_id - - # The user has access for every partition, return none - return None - def check_group_access(self, user_groups): """ Arguments: @@ -277,4 +220,21 @@ class _MergedGroupAccess(object): Returns: bool: Whether said user has group access. """ - return self.get_access_denying_partition(user_groups) is None + for partition_id, allowed_group_ids in self._access.iteritems(): + + # If the user is not assigned to a group for this partition, + # deny access. + if partition_id not in user_groups: + return False + + # If the user belongs to one of the allowed groups for this + # partition, then move and check the next partition. + elif user_groups[partition_id].id in allowed_group_ids: + continue + + # Else, deny access. + else: + return False + + # The user has access for every partition, grant access. + return True diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 60391f3fb0..ccdac8abc3 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -503,12 +503,11 @@ def _has_group_access(descriptor, user, course_key): if missing_groups: partition, user_group, allowed_groups = missing_groups[0] - block_key = descriptor.scope_ids.usage_id return IncorrectPartitionGroupError( partition=partition, user_group=user_group, allowed_groups=allowed_groups, - user_message=partition.access_denied_message(block_key, user, user_group, allowed_groups), + user_message=partition.access_denied_message(descriptor, user, user_group, allowed_groups), user_fragment=partition.access_denied_fragment(descriptor, user, user_group, allowed_groups), ) diff --git a/openedx/features/content_type_gating/partitions.py b/openedx/features/content_type_gating/partitions.py index b640a19fab..3b2e03b5f1 100644 --- a/openedx/features/content_type_gating/partitions.py +++ b/openedx/features/content_type_gating/partitions.py @@ -85,8 +85,8 @@ class ContentTypeGatingPartition(UserPartition): if (verified_mode is None or not self._is_audit_enrollment(user, course_key) or user_group == FULL_ACCESS): return None - ecommerce_checkout_link = self._get_checkout_link(user, verified_mode.sku) + request = crum.get_current_request() frag = Fragment(render_to_string('content_type_gating/access_denied_message.html', { 'mobile_app': request and is_request_from_mobile_app(request), @@ -95,19 +95,14 @@ class ContentTypeGatingPartition(UserPartition): })) return frag - def access_denied_message(self, block_key, user, user_group, allowed_groups): - course_key = block_key.course_key + def access_denied_message(self, block, user, user_group, allowed_groups): + course_key = self._get_course_key_from_course_block(block) modes = CourseMode.modes_for_course_dict(course_key) verified_mode = modes.get(CourseMode.VERIFIED) if (verified_mode is None or not self._is_audit_enrollment(user, course_key) or user_group == FULL_ACCESS): return None - - request = crum.get_current_request() - if request and is_request_from_mobile_app(request): - return _(u"Graded assessments are available to Verified Track learners.") - else: - return _(u"Graded assessments are available to Verified Track learners. Upgrade to Unlock.") + return _(u"Graded assessments are available to Verified Track learners. Upgrade to Unlock.") def _is_audit_enrollment(self, user, course_key): """ diff --git a/openedx/features/content_type_gating/tests/test_partitions.py b/openedx/features/content_type_gating/tests/test_partitions.py index de08ef914e..641ab1a25a 100644 --- a/openedx/features/content_type_gating/tests/test_partitions.py +++ b/openedx/features/content_type_gating/tests/test_partitions.py @@ -119,7 +119,7 @@ class TestContentTypeGatingPartition(CacheIsolationTestCase): ): fragment = partition.access_denied_fragment(mock_block, global_staff, FULL_ACCESS, 'test_allowed_group') self.assertIsNone(fragment) - message = partition.access_denied_message(mock_block.scope_ids.usage_id, global_staff, FULL_ACCESS, 'test_allowed_group') + message = partition.access_denied_message(mock_block, global_staff, FULL_ACCESS, 'test_allowed_group') self.assertIsNone(message) def test_acess_denied_fragment_for_null_request(self): diff --git a/setup.py b/setup.py index 8e9822aaba..26e0426552 100644 --- a/setup.py +++ b/setup.py @@ -63,7 +63,6 @@ setup( "completion = lms.djangoapps.course_api.blocks.transformers.block_completion:BlockCompletionTransformer", "load_override_data = lms.djangoapps.course_blocks.transformers.load_override_data:OverrideDataTransformer", "content_type_gate = openedx.features.content_type_gating.block_transformers:ContentTypeGateTransformer", - "access_denied_message_filter = lms.djangoapps.course_blocks.transformers.access_denied_filter:AccessDeniedMessageFilterTransformer", ], "openedx.ace.policy": [ "bulk_email_optout = lms.djangoapps.bulk_email.policies:CourseEmailOptout"