Merge pull request #24302 from cpennington/prioritize-access-denied-messages

Prioritize access denied messages

lms/djangoapps/course_blocks/transformers/user_partitions.py

@@ -20,7 +20,7 @@ from .split_test import SplitTestTransformer
 from .utils import get_field_on_block
 
 
-class UserPartitionTransformer(FilteringTransformerMixin, BlockStructureTransformer):
+class UserPartitionTransformer(BlockStructureTransformer):
     """
     A transformer that enforces the group access rules on course blocks,
     by honoring their user_partitions and group_access fields, and
@@ -78,18 +78,18 @@ class UserPartitionTransformer(FilteringTransformerMixin, BlockStructureTransfor
             merged_group_access = _MergedGroupAccess(user_partitions, xblock, merged_parent_access_list)
             block_structure.set_transformer_block_field(block_key, cls, 'merged_group_access', merged_group_access)
 
-    def transform_block_filters(self, usage_info, block_structure):
+    def transform(self, usage_info, block_structure):
         user = usage_info.user
-        result_list = SplitTestTransformer().transform_block_filters(usage_info, block_structure)
+        SplitTestTransformer().transform(usage_info, block_structure)
         staff_access = has_access(user, 'staff', usage_info.course_key)
 
         # If you have staff access, you are allowed access to the entire result list
         if staff_access:
-            return result_list
+            return
 
         user_partitions = block_structure.get_transformer_data(self, 'user_partitions')
         if not user_partitions:
-            return [block_structure.create_universal_filter()]
+            return
 
         user_groups = get_user_partition_groups(usage_info.course_key, user_partitions, user, 'id')
 
@@ -97,35 +97,38 @@ class UserPartitionTransformer(FilteringTransformerMixin, BlockStructureTransfor
             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(
+            if transformer_block_field is None:
+                continue
+
+            access_denying_partition_ids = transformer_block_field.get_access_denying_partitions(
                 user_groups
             )
-            access_denying_partition = get_partition_from_id(user_partitions, access_denying_partition_id)
+            access_denying_reasons = []
+            access_denying_messages = []
+            for partition_id in access_denying_partition_ids:
+                access_denying_partition = get_partition_from_id(user_partitions, partition_id)
 
-            if access_denying_partition:
+                if access_denying_partition:
-                user_group = user_groups.get(access_denying_partition.id)
+                    user_group = user_groups.get(access_denying_partition.id)
-                allowed_groups = transformer_block_field.get_allowed_groups()[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(
+                    access_denied_message = access_denying_partition.access_denied_message(
-                    block_key, user, user_group, allowed_groups
+                        block_key, user, user_group, allowed_groups
-                )
+                    )
-                block_structure.override_xblock_field(
+                    access_denying_reasons.append(access_denying_partition.name)
-                    block_key, 'authorization_denial_reason', access_denying_partition.name
+                    if access_denied_message:
-                )
+                        access_denying_messages.append(access_denied_message)
-                block_structure.override_xblock_field(
-                    block_key, 'authorization_denial_message', access_denied_message
-                )
 
-        group_access_filter = block_structure.create_removal_filter(
+            if access_denying_reasons and not access_denying_messages:
-            lambda block_key: (
+                block_structure.remove_block(block_key, keep_descendants=False)
-                block_structure.get_transformer_block_field(
+            else:
-                    block_key, self, 'merged_group_access'
+                if access_denying_reasons:
-                ).get_access_denying_partition(user_groups) is not None and
+                    block_structure.override_xblock_field(
-                block_structure.get_xblock_field(block_key, 'authorization_denial_message') is None
+                        block_key, 'authorization_denial_reason', access_denying_reasons[0]
-            )
+                    )
-        )
+                if access_denying_messages:
-        result_list.append(group_access_filter)
+                    block_structure.override_xblock_field(
-
+                        block_key, 'authorization_denial_message', access_denying_messages[0]
-        return result_list
+                    )
 
 
 class _MergedGroupAccess(object):
@@ -248,7 +251,7 @@ class _MergedGroupAccess(object):
         else:
             return None
 
-    def get_access_denying_partition(self, user_groups):
+    def get_access_denying_partitions(self, user_groups):
         """
         Arguments:
             dict[int: Group]: Given a user, a mapping from user
@@ -256,25 +259,21 @@ class _MergedGroupAccess(object):
                 each partition.
 
         Returns:
-            bool: Which partition is denying access
+            list of ints: Which partition is denying access
         """
-        for partition_id, allowed_group_ids in six.iteritems(self._access):
+        denied_access = []
+        for partition_id, allowed_group_ids in self.get_allowed_groups().items():
             # If the user is not assigned to a group for this partition,
-            # return partition that would deny access.
+            # return partition as one that would deny access.
             if partition_id not in user_groups:
-                return partition_id
+                denied_access.append(partition_id)
 
-            # If the user belongs to one of the allowed groups for this
+            # If the user does not belong to one of the allowed groups for this
-            # partition, then move and check the next partition.
+            # partition, then return this partition as one that would deny access
-            elif user_groups[partition_id].id in allowed_group_ids:
+            elif user_groups[partition_id].id not in allowed_group_ids:
-                continue
+                denied_access.append(partition_id)
 
-            # Else, return partition that would deny access.
+        return denied_access
-            else:
-                return partition_id
-
-        # The user has access for every partition, return none
-        return None
 
     def check_group_access(self, user_groups):
         """
@@ -286,4 +285,4 @@ class _MergedGroupAccess(object):
         Returns:
             bool: Whether said user has group access.
         """
-        return self.get_access_denying_partition(user_groups) is None
+        return not self.get_access_denying_partitions(user_groups)

lms/djangoapps/courseware/access.py

@@ -502,6 +502,7 @@ def _has_group_access(descriptor, user, course_key):
     # If missing_groups is empty, the user is granted access.
     # If missing_groups is NOT empty, we generate an error based on one of the particular groups they are missing.
     missing_groups = []
+    block_key = descriptor.scope_ids.usage_id
     for partition, groups in partition_groups:
         user_group = partition.scheme.get_group_for_user(
             course_key,
@@ -509,17 +510,25 @@ def _has_group_access(descriptor, user, course_key):
             partition,
         )
         if user_group not in groups:
-            missing_groups.append((partition, user_group, groups))
+            missing_groups.append((
+                partition,
+                user_group,
+                groups,
+                partition.access_denied_message(block_key, user, user_group, groups),
+                partition.access_denied_fragment(descriptor, user, user_group, groups),
+            ))
 
     if missing_groups:
-        partition, user_group, allowed_groups = missing_groups[0]
+        # Prefer groups with explanatory messages
-        block_key = descriptor.scope_ids.usage_id
+        # False < True, so the default order and `is None` results in groups with messages coming first
+        ordered_groups = sorted(missing_groups, key=lambda details: (details[3] is None, details[4] is None))
+        partition, user_group, allowed_groups, message, fragment = ordered_groups[0]
         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=message,
-            user_fragment=partition.access_denied_fragment(descriptor, user, user_group, allowed_groups),
+            user_fragment=fragment,
         )
 
     # all checks passed.

openedx/core/djangoapps/content/block_structure/transformer.py

@@ -5,6 +5,7 @@ Transformers.
 
 
 from abc import abstractmethod
+import functools
 
 
 class BlockStructureTransformer(object):
@@ -177,7 +178,8 @@ class FilteringTransformerMixin(BlockStructureTransformer):
         transform_block_filters calls will be combined and used in a single
         tree traversal.
         """
-        block_structure.filter_topological_traversal(self.transform_block_filters(usage_info, block_structure))
+        filters = self.transform_block_filters(usage_info, block_structure)
+        block_structure.filter_topological_traversal(combine_filters(block_structure, filters))
 
     @abstractmethod
     def transform_block_filters(self, usage_info, block_structure):
@@ -209,3 +211,19 @@ class FilteringTransformerMixin(BlockStructureTransformer):
                 transformer, that is to be transformed in place.
         """
         raise NotImplementedError
+
+
+def combine_filters(block_structure, filters):
+    return functools.reduce(
+        _filter_chain,
+        filters,
+        block_structure.create_universal_filter()
+    )
+
+
+def _filter_chain(accumulated, additional):
+    """
+    Given two functions that take a block_key and return a boolean, yield
+    a function that takes a block key, and 'ands' the functions together
+    """
+    return lambda block_key: accumulated(block_key) and additional(block_key)

openedx/core/djangoapps/content/block_structure/transformers.py

@@ -7,7 +7,7 @@ import functools
 from logging import getLogger
 
 from .exceptions import TransformerDataIncompatible, TransformerException
-from .transformer import FilteringTransformerMixin
+from .transformer import FilteringTransformerMixin, combine_filters
 from .transformer_registry import TransformerRegistry
 
 logger = getLogger(__name__)  # pylint: disable=C0103
@@ -131,20 +131,9 @@ class BlockStructureTransformers(object):
         for transformer in self._transformers['supports_filter']:
             filters.extend(transformer.transform_block_filters(self.usage_info, block_structure))
 
-        combined_filters = functools.reduce(
+        combined_filters = combine_filters(block_structure, filters)
-            self._filter_chain,
-            filters,
-            block_structure.create_universal_filter()
-        )
         block_structure.filter_topological_traversal(combined_filters)
 
-    def _filter_chain(self, accumulated, additional):
-        """
-        Given two functions that take a block_key and return a boolean, yield
-        a function that takes a block key, and 'ands' the functions together
-        """
-        return lambda block_key: accumulated(block_key) and additional(block_key)
-
     def _transform_without_filters(self, block_structure):
         """
         Transforms the given block_structure using the transform