diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 1f42a559bd..d02b90a7a0 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -95,7 +95,7 @@ def has_ccx_coach_role(user, course_key): return False -def has_access(user, action, obj, course_key=None, prefetched_group_data=None): +def has_access(user, action, obj, course_key=None): """ Check whether a user has the access to do action on obj. Handles any magic switching based on various settings. @@ -150,7 +150,7 @@ def has_access(user, action, obj, course_key=None, prefetched_group_data=None): # NOTE: any descriptor access checkers need to go above this if isinstance(obj, XBlock): - return _has_access_descriptor(user, action, obj, course_key, prefetched_group_data) + return _has_access_descriptor(user, action, obj, course_key) if isinstance(obj, CourseKey): return _has_access_course_key(user, action, obj) @@ -399,21 +399,7 @@ def _has_access_error_desc(user, action, descriptor, course_key): return _dispatch(checkers, action, user, descriptor) -def _partition_and_user_groups_prefetch(prefetched_group_data, merged_access): - """Compute the needed partition_groups and user_groups structures using prefetched data.""" - partition_groups = { - partition_id: set(group_ids).intersection( - set(prefetched_group_data.get(partition_id, {}).get('all_group_ids', [])) - ) for partition_id, group_ids in merged_access.iteritems() - } - user_groups = { - partition_id: prefetched_group_data.get(partition_id, {}).get('user_group_id') - for partition_id in merged_access.iterkeys() - } - return partition_groups, user_groups - - -def _has_group_access(descriptor, user, course_key, prefetched_group_data): +def _has_group_access(descriptor, user, course_key): """ This function returns a boolean indicating whether or not `user` has sufficient group memberships to "load" a block (the `descriptor`) @@ -431,63 +417,60 @@ def _has_group_access(descriptor, user, course_key, prefetched_group_data): log.warning("Group access check excludes all students, access will be denied.", exc_info=True) return ACCESS_DENIED - partition_groups = {} - user_groups = {} - if prefetched_group_data: - # Use prefetched data, if possible. See EDUCATOR-2618 for context. - partition_groups, user_groups = _partition_and_user_groups_prefetch(prefetched_group_data, merged_access) - else: - # resolve the partition IDs in group_access to actual - # partition objects, skipping those which contain empty group directives. - # If a referenced partition could not be found, it will be denied - # If the partition is found but is no longer active (meaning it's been disabled) - # then skip the access check for that partition. - partitions = [] - for partition_id, group_ids in merged_access.iteritems(): - try: - partition = descriptor._get_user_partition(partition_id) # pylint: disable=protected-access - if partition.active: - if group_ids is not None: - partitions.append(partition) - else: - log.debug( - "Skipping partition with ID %s in course %s because it is no longer active", - partition.id, course_key - ) - except NoSuchUserPartitionError: - log.warning("Error looking up user partition, access will be denied.", exc_info=True) - return ACCESS_DENIED - - # next resolve the group IDs specified within each partition + # resolve the partition IDs in group_access to actual + # partition objects, skipping those which contain empty group directives. + # If a referenced partition could not be found, it will be denied + # If the partition is found but is no longer active (meaning it's been disabled) + # then skip the access check for that partition. + partitions = [] + for partition_id, group_ids in merged_access.items(): try: - for partition in partitions: - groups = [ - partition.get_group(group_id) - for group_id in merged_access[partition.id] - ] - if groups: - partition_groups[partition.id] = [group.id for group in groups] - # look up the user's group for each partition - user_group = partition.scheme.get_group_for_user( - course_key, - user, - partition, - ) - user_groups[partition.id] = user_group.id if user_group else None - except NoSuchUserPartitionGroupError: - log.warning("Error looking up referenced user partition group, access will be denied.", exc_info=True) + partition = descriptor._get_user_partition(partition_id) # pylint: disable=protected-access + if partition.active: + if group_ids is not None: + partitions.append(partition) + else: + log.debug( + "Skipping partition with ID %s in course %s because it is no longer active", + partition.id, course_key + ) + except NoSuchUserPartitionError: + log.warning("Error looking up user partition, access will be denied.", exc_info=True) return ACCESS_DENIED + # next resolve the group IDs specified within each partition + partition_groups = [] + try: + for partition in partitions: + groups = [ + partition.get_group(group_id) + for group_id in merged_access[partition.id] + ] + if groups: + partition_groups.append((partition, groups)) + except NoSuchUserPartitionGroupError: + log.warning("Error looking up referenced user partition group, access will be denied.", exc_info=True) + return ACCESS_DENIED + + # look up the user's group for each partition + user_groups = {} + for partition, groups in partition_groups: + user_groups[partition.id] = partition.scheme.get_group_for_user( + course_key, + user, + partition, + ) + # finally: check that the user has a satisfactory group assignment # for each partition. - if not all(user_groups.get(partition_id) in group_ids for partition_id, group_ids in partition_groups.iteritems()): + if not all(user_groups.get(partition.id) in groups for partition, groups in partition_groups): return ACCESS_DENIED # all checks passed. return ACCESS_GRANTED -def _has_access_descriptor(user, action, descriptor, course_key=None, prefetched_group_data=None): +def _has_access_descriptor(user, action, descriptor, course_key=None): """ Check if user has access to this descriptor. @@ -510,7 +493,7 @@ def _has_access_descriptor(user, action, descriptor, course_key=None, prefetched # access to this content, then deny access. The problem with calling _has_staff_access_to_descriptor # before this method is that _has_staff_access_to_descriptor short-circuits and returns True # for staff users in preview mode. - if not _has_group_access(descriptor, user, course_key, prefetched_group_data): + if not _has_group_access(descriptor, user, course_key): return ACCESS_DENIED # If the user has staff access, they can load the module and checks below are not needed. diff --git a/lms/djangoapps/django_comment_client/utils.py b/lms/djangoapps/django_comment_client/utils.py index 88618ebd54..24c928c349 100644 --- a/lms/djangoapps/django_comment_client/utils.py +++ b/lms/djangoapps/django_comment_client/utils.py @@ -4,7 +4,7 @@ from collections import defaultdict from datetime import datetime from django.conf import settings -from django.contrib.auth.models import AnonymousUser, User +from django.contrib.auth.models import User from django.core.urlresolvers import reverse from django.db import connection from django.http import HttpResponse @@ -131,45 +131,13 @@ def get_accessible_discussion_xblocks_by_course_id(course_id, user=None, include Checks for the given user's access if include_all is False. """ all_xblocks = modulestore().get_items(course_id, qualifiers={'category': 'discussion'}, include_orphans=False) - if not all_xblocks: - return [] - prefetched_group_data = prefetch_group_data(user, all_xblocks[0], course_id) return [ xblock for xblock in all_xblocks - if ( - has_required_keys(xblock) and ( - include_all or - has_access(user, 'load', xblock, course_id, prefetched_group_data=prefetched_group_data) - ) - ) + if has_required_keys(xblock) and (include_all or has_access(user, 'load', xblock, course_id)) ] -def prefetch_group_data(user, sample_block, course_id): - """ - Return a dict containing: - - id of every partition in the course, mapped to: - - all of their corresponding group_ids - - the group_id appropriate for user - """ - all_partitions = sample_block.runtime.service(sample_block, 'partitions').course_partitions - if not user: - user = AnonymousUser() - - def _group_id_or_none(partition, course_id, user): - """If an appropriate group for this user exists, return its id. Else return None.""" - group = partition.scheme.get_group_for_user(course_id, user, partition) - return group.id if group else None - - return { - partition.id: { - 'all_group_ids': [group.id for group in partition.groups], - 'user_group_id': _group_id_or_none(partition, course_id, user), - } for partition in all_partitions if partition.active - } - - def get_discussion_id_map_entry(xblock): """ Returns a tuple of (discussion_id, metadata) suitable for inclusion in the results of get_discussion_id_map().