Ensure hidden child elements are not returned in queryset

backend/src/baserow/contrib/builder/elements/permission_manager.py

@@ -1,5 +1,5 @@
 from django.contrib.auth import get_user_model
-from django.db.models import Q
+from django.db.models import Q, QuerySet
 
 from baserow.contrib.builder.elements.operations import ListElementsPageOperationType
 from baserow.contrib.builder.workflow_actions.operations import (
@@ -15,6 +15,13 @@ from .models import Element
 User = get_user_model()
 
 
+# For now there can be up to three levels of nested elements.
+# E.g. a RepeatElement might contain a ColumnElement, which might contain a
+# HeadingElement.
+# However, later this number could be dynamic depending on the page itself.
+MAX_ELEMENT_NESTING_DEPTH = 3
+
+
 class ElementVisibilityPermissionManager(PermissionManagerType):
     """This permission manager handle the element visibility permissions."""
 
@@ -104,6 +111,96 @@ class ElementVisibilityPermissionManager(PermissionManagerType):
 
         return result
 
+    def exclude_elements_with_role(
+        self,
+        queryset: QuerySet,
+        role_type: Element.ROLE_TYPES,
+        role: str,
+        prefix: str = "",
+    ) -> QuerySet:
+        """
+        Update the queryset by excluding all Elements that match a particular
+        role_type *and* role.
+
+        The prefix is to support Elements that are a child of a different
+        Instance, e.g. when a BuilderWorkflowAction queryset is passed in,
+        we want to filter against the Element foreign key, not the action
+        itself.
+
+        The queryset exclusion logic is repeated to support the maximum level
+        of element nesting.
+        """
+
+        query = Q()
+        for level in range(MAX_ELEMENT_NESTING_DEPTH):
+            path = prefix + "parent_element__" * level
+            query |= Q(**{f"{path}role_type": role_type}) & Q(
+                **{f"{path}roles__contains": role}
+            )
+
+        queryset = queryset.exclude(query)
+
+        return queryset
+
+    def exclude_elements_without_role(
+        self,
+        queryset: QuerySet,
+        role_type: Element.VISIBILITY_TYPES,
+        role: str,
+        prefix: str = "",
+    ) -> QuerySet:
+        """
+        Update the queryset by excluding all Elements that match a particular
+        role_type but *not* the role.
+
+        The prefix is to support Elements that are a child of a different
+        Instance, e.g. when a BuilderWorkflowAction queryset is passed in,
+        we want to filter against the Element foreign key, not the action
+        itself.
+
+        The queryset exclusion logic is repeated to support the maximum level
+        of element nesting.
+        """
+
+        query = Q()
+        for level in range(MAX_ELEMENT_NESTING_DEPTH):
+            path = prefix + "parent_element__" * level
+            query |= Q(**{f"{path}role_type": role_type}) & ~Q(
+                **{f"{path}roles__contains": role}
+            )
+
+        queryset = queryset.exclude(query)
+
+        return queryset
+
+    def exclude_elements_with_visibility(
+        self,
+        queryset: QuerySet,
+        visibility_type: Element.VISIBILITY_TYPES,
+        prefix: str = "",
+    ) -> QuerySet:
+        """
+        Update the queryset by excluding all Elements that match a particular
+        visibility_type.
+
+        The prefix is to support Elements that are a child of a different
+        Instance, e.g. when a BuilderWorkflowAction instance is passed in
+        we want to filter against its element foreign key, not the action
+        itself.
+
+        The queryset exclusion logic is repeated to support the maximum level
+        of element nesting.
+        """
+
+        query = Q()
+        for level in range(MAX_ELEMENT_NESTING_DEPTH):
+            path = prefix + "parent_element__" * level
+            query |= Q(**{f"{path}visibility": visibility_type})
+
+        queryset = queryset.exclude(query)
+
+        return queryset
+
     def filter_queryset(
         self,
         actor,
@@ -118,40 +215,44 @@ class ElementVisibilityPermissionManager(PermissionManagerType):
 
         if operation_name == ListElementsPageOperationType.type:
             if getattr(actor, "is_authenticated", False):
-                queryset = (
-                    queryset.exclude(visibility=Element.VISIBILITY_TYPES.NOT_LOGGED)
-                    .exclude(
-                        Q(role_type=Element.ROLE_TYPES.ALLOW_ALL_EXCEPT)
-                        & Q(roles__contains=actor.role)
-                    )
-                    .exclude(
-                        Q(role_type=Element.ROLE_TYPES.DISALLOW_ALL_EXCEPT)
-                        & ~Q(roles__contains=actor.role)
-                    )
+                queryset = self.exclude_elements_with_visibility(
+                    queryset, Element.VISIBILITY_TYPES.NOT_LOGGED
+                )
+                queryset = self.exclude_elements_with_role(
+                    queryset, Element.ROLE_TYPES.ALLOW_ALL_EXCEPT, actor.role
+                )
+                queryset = self.exclude_elements_without_role(
+                    queryset, Element.ROLE_TYPES.DISALLOW_ALL_EXCEPT, actor.role
                 )
-                return queryset
-            else:
-                return queryset.exclude(visibility=Element.VISIBILITY_TYPES.LOGGED_IN)
 
-        elif operation_name == ListBuilderWorkflowActionsPageOperationType.type:
-            if getattr(actor, "is_authenticated", False):
-                queryset = (
-                    queryset.exclude(
-                        element__visibility=Element.VISIBILITY_TYPES.NOT_LOGGED
-                    )
-                    .exclude(
-                        Q(element__role_type=Element.ROLE_TYPES.ALLOW_ALL_EXCEPT)
-                        & Q(element__roles__contains=actor.role)
-                    )
-                    .exclude(
-                        Q(element__role_type=Element.ROLE_TYPES.DISALLOW_ALL_EXCEPT)
-                        & ~Q(element__roles__contains=actor.role)
-                    )
-                )
                 return queryset
             else:
-                return queryset.exclude(
-                    element__visibility=Element.VISIBILITY_TYPES.LOGGED_IN
+                return self.exclude_elements_with_visibility(
+                    queryset, Element.VISIBILITY_TYPES.LOGGED_IN
+                )
+        elif operation_name == ListBuilderWorkflowActionsPageOperationType.type:
+            prefix = "element__"
+            if getattr(actor, "is_authenticated", False):
+                queryset = self.exclude_elements_with_visibility(
+                    queryset, Element.VISIBILITY_TYPES.NOT_LOGGED, prefix=prefix
+                )
+                queryset = self.exclude_elements_with_role(
+                    queryset,
+                    Element.ROLE_TYPES.ALLOW_ALL_EXCEPT,
+                    actor.role,
+                    prefix=prefix,
+                )
+                queryset = self.exclude_elements_without_role(
+                    queryset,
+                    Element.ROLE_TYPES.DISALLOW_ALL_EXCEPT,
+                    actor.role,
+                    prefix=prefix,
+                )
+
+                return queryset
+            else:
+                return self.exclude_elements_with_visibility(
+                    queryset, Element.VISIBILITY_TYPES.LOGGED_IN, prefix=prefix
                 )
 
         return queryset

backend/tests/baserow/contrib/builder/elements/test_element_permission_manager.py

@@ -619,6 +619,141 @@ def test_queryset_only_includes_elements_allowed_by_role(
         assert len(elements) == element_count
 
 
+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    "user_role,parent_element_roles,chid_element_roles,parent_visibility_type,child_visibility_type,role_type,logged_in",
+    [
+        (
+            "",
+            [],
+            [],
+            # Parent Element's visibility should be logged in, thus is
+            # invisible to Anon users.
+            Element.VISIBILITY_TYPES.LOGGED_IN,
+            # Child Element's visibility should be 'not logged', since we want
+            # to test if the child element is visible to Anon users.
+            Element.VISIBILITY_TYPES.NOT_LOGGED,
+            Element.ROLE_TYPES.DISALLOW_ALL_EXCEPT,
+            False,
+        ),
+        (
+            "user_role",
+            ["parent_role"],
+            ["child_role"],
+            Element.VISIBILITY_TYPES.NOT_LOGGED,
+            Element.VISIBILITY_TYPES.NOT_LOGGED,
+            # The role type here doesn't matter, since the element
+            # is visible to non-logged in users.
+            Element.ROLE_TYPES.DISALLOW_ALL_EXCEPT,
+            True,
+        ),
+        (
+            "user_role",
+            # Parent shouldn't allow user_role to see it
+            ["user_role"],
+            # Child should allow user_role to see it
+            [],
+            Element.VISIBILITY_TYPES.LOGGED_IN,
+            Element.VISIBILITY_TYPES.LOGGED_IN,
+            Element.ROLE_TYPES.ALLOW_ALL_EXCEPT,
+            True,
+        ),
+        (
+            "user_role",
+            # Parent element shouldn't allow anyone access to it
+            [],
+            # Child should allow user_role to see it
+            ["user_role"],
+            Element.VISIBILITY_TYPES.LOGGED_IN,
+            Element.VISIBILITY_TYPES.LOGGED_IN,
+            Element.ROLE_TYPES.DISALLOW_ALL_EXCEPT,
+            True,
+        ),
+    ],
+)
+def test_queryset_excludes_all_child_elements(
+    ab_builder_user_page,
+    data_fixture,
+    user_role,
+    parent_element_roles,
+    chid_element_roles,
+    parent_visibility_type,
+    child_visibility_type,
+    role_type,
+    logged_in,
+):
+    """
+    Ensure that if the parent element is hidden due to a role, all its child
+    elements are excluded from the returned queryset.
+    """
+
+    public_user_source, _, public_page = ab_builder_user_page
+
+    public_user_source_user = UserSourceUser(
+        public_user_source,
+        None,
+        1,
+        "foo_username",
+        "foo@bar.com",
+        user_role,
+    )
+
+    # Create a Repeat element, which will be the parent element and first
+    # level of nesting.
+    repeat_element = data_fixture.create_builder_repeat_element(
+        page=public_page,
+        visibility=parent_visibility_type,
+        roles=parent_element_roles,
+        role_type=role_type,
+    )
+
+    # Create a Column element, which is the 2nd level of nesting.
+    column_element = data_fixture.create_builder_column_element(
+        page=public_page,
+        visibility=child_visibility_type,
+        roles=chid_element_roles,
+        role_type=role_type,
+        parent_element_id=repeat_element.id,
+    )
+
+    # Add a Heading element that matches the user's role, and is the final
+    # 3rd level of nesting.
+    element = data_fixture.create_builder_heading_element(
+        page=public_page,
+        visibility=child_visibility_type,
+        roles=chid_element_roles,
+        role_type=role_type,
+        parent_element_id=column_element.id,
+    )
+
+    # Create a workflow action connected to the element that requires the role
+    workflow_action_logged_in = (
+        data_fixture.create_local_baserow_create_row_workflow_action(
+            page=public_page, element=element
+        )
+    )
+
+    perm_manager = ElementVisibilityPermissionManager()
+
+    user = public_user_source_user
+    if not logged_in:
+        user = AnonymousUser()
+
+    elements = perm_manager.filter_queryset(
+        user,
+        ListElementsPageOperationType.type,
+        Element.objects.all(),
+    )
+    assert len(elements) == 0
+
+    actions = perm_manager.filter_queryset(
+        user,
+        ListBuilderWorkflowActionsPageOperationType.type,
+        BuilderWorkflowAction.objects.all(),
+    )
+    assert len(actions) == 0
+
+
 @pytest.mark.django_db
 @pytest.mark.parametrize(
     "role,roles,role_type,expected_bool_result",

changelog/entries/unreleased/bug/2601_builder_fix_bug_where_some_child_elements_that_should_have_b.json

@@ -0,0 +1,7 @@
+{
+    "type": "bug",
+    "message": "[Builder] Fix bug where some child elements that should have been excluded by the Permission Manager were being returned in the queryset.",
+    "issue_number": 2601,
+    "bullet_points": [],
+    "created_at": "2024-07-19"
+}