Skip to content

fix: enhance folder permission checks by incorporating user group permissions #7884

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wxg0103 merged 1 commit into from
Nov 13, 2026
Merged

fix: enhance folder permission checks by incorporating user group permissions #7884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 18 additions & 9 deletions apps/folders/serializers/folder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
from knowledge.models import KnowledgeFolder, Knowledge
from knowledge.serializers.knowledge import KnowledgeSerializer
from knowledge.serializers.knowledge_folder import KnowledgeFolderTreeSerializer
from system_manage.models import WorkspaceUserResourcePermission
from system_manage.models import WorkspaceUserGroupResourcePermission, WorkspaceUserResourcePermission
from system_manage.serializers.user_resource_permission import UserResourcePermissionSerializer
from tools.models import ToolFolder, Tool
from tools.serializers.tool import ToolSerializer
Expand Down Expand Up @@ -453,14 +453,23 @@ def get_folder_tree(self, current_user, name=None):
if having_read_permission_by_role:
permission_condition = ["VIEW", "ROLE"]

base_q &= Q(
id__in=WorkspaceUserResourcePermission.objects.filter(
user_id=current_user.id,
auth_target_type=self.data.get("source"),
workspace_id=self.data.get("workspace_id"),
permission_list__overlap=permission_condition,
).values_list("target", flat=True)
) | Q(id=self.data.get("workspace_id"))
user_authorized_folder_ids = WorkspaceUserResourcePermission.objects.filter(
user_id=current_user.id,
auth_target_type=self.data.get("source"),
workspace_id=self.data.get("workspace_id"),
permission_list__overlap=permission_condition,
).values_list("target", flat=True)
group_authorized_folder_ids = WorkspaceUserGroupResourcePermission.objects.filter(
user_group__user_relations__user_id=current_user.id,
auth_target_type=self.data.get("source"),
workspace_id=self.data.get("workspace_id"),
permission_list__overlap=permission_condition,
).values_list("target", flat=True)
base_q &= (
Q(id__in=user_authorized_folder_ids)
| Q(id__in=group_authorized_folder_ids)
| Q(id=self.data.get("workspace_id"))
)

nodes = Folder.objects.filter(base_q).get_cached_trees()

@shaohuzhang1 shaohuzhang1 Nov 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an analysis of the code changes, highlighting potential issues, logic checks, and optimization suggestions.

1. Logic & Correctness

  • Correct Permission Union: The change correctly implements a union of permissions. A user now has access if they have direct WorkspaceUserResourcePermission OR via GroupMembership (user_group__user_relations). This is standard RBAC (Role-Based Access Control) logic.
  • permission_condition Scope: Ensure that the variable permission_condition (a list like ["VIEW", "ROLE"]) is correctly defined inside the having_read_permission_by_role block and accessible when building the queries for both users and groups. If it's defined conditionally, ensure the subsequent lines are only executed when appropriate.
  • values_list Efficiency: Using .values_list("target", flat=True) is efficient because it avoids instantiating model objects.

2. Potential Issues

  • N+1 Query / Subquery Overhead: While not an N+1 issue in the traditional loop sense, chaining these subqueries can lead to complex SQL plans. However, Django ORM usually generates a single query with OR conditions and subqueries, which is acceptable.
  • Indexing: For performance, ensure that database indexes exist on:
    • WorkspaceUserResourcePermission(user_id, auth_target_type, workspace_id)
    • WorkspaceUserGroupResourcePermission(user_group, auth_target_type, workspace_id)
    • Any intermediate join tables for user_group__user_relations.
  • Empty Querysets: If current_user has no direct or group permissions, the Q(id__in=...) clauses will filter nothing, but the | Q(id=self.data.get("workspace_id")) ensures at least the workspace root is included (assuming that’s intended behavior). Verify if users with no explicit permissions should indeed see the workspace root or not. Often, workspace visibility is controlled separately.

3. Optimization & Code Quality Suggestions

  • Variable Naming: Clear and descriptive. Good.
  • DRY Principle: There is some repetition in filtering by auth_target_type and workspace_id. You could refactor slightly for readability, though the current version is clear enough.
  • Query Combination: Instead of two separate values_list calls followed by combining Q objects, you could potentially combine the queries before evaluating. However, the current approach is fine. Here is a slightly more optimized pattern using a single combined query if the models support it directly, but since they are different models, separate queries are necessary.

Suggested Refactor (Minor Readability Improvement)

Current code is fine, but wrapping the permission filters into helper methods or variables if used elsewhere would be better. For this snippet, it's okay as-is.

4. Final Verdict

✅ No critical bugs detected.
⚠️ Check Indexes: Ensure DB indexes to support the new WorkspaceUserGroupResourcePermission lookups, especially traversing relationships (user_group__user_relations__user_id).
💡 Logic Check: Confirm business logic allows default visibility to workspace_id even without explicit folder permissions.

Optimized Version (Optional)

If you want to slightly tighten the syntax:

# Inside get_folder_tree after defining permission_condition

direct_perms = WorkspaceUserResourcePermission.objects.filter(
    user_id=current_user.id,
    auth_target_type=self.data.get("source"),
    workspace_id=self.data.get("workspace_id"),
    permission_list__overlap=permission_condition,
).values_list("target", flat=True)

group_perms = WorkspaceUserGroupResourcePermission.objects.filter(
    user_group__user_relations__user_id=current_user.id,
    auth_target_type=self.data.get("source"),
    workspace_id=self.data.get("workspace_id"),
    permission_list__overlap=permission_condition,
).values_list("target", flat=True)

base_q &= (
    Q(id__in=direct_perms) | 
    Q(id__in=group_perms) | 
    Q(id=self.data.get("workspace_id"))
)

This matches your current logic exactly. No functional change needed unless indexing is missing.

Expand Down
67 changes: 67 additions & 0 deletions apps/folders/tests/test_folder_tree_permission.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
"""Folder tree honors permissions granted through user groups."""

from types import SimpleNamespace
from unittest.mock import patch

from application.models import ApplicationFolder
from django.test import TestCase
from knowledge.models import KnowledgeFolder
from system_manage.models import WorkspaceUserGroupResourcePermission
from tools.models import ToolFolder
from users.models import User
from users.models.user_group import SystemUserGroup, SystemUserGroupRelation

from folders.serializers.folder import FolderTreeSerializer


class FolderTreeUserGroupPermissionTests(TestCase):
workspace_id = "00000000-0000-0000-0000-000000000010"
user_id = "00000000-0000-0000-0000-000000000011"
group_id = "00000000-0000-0000-0000-000000000012"
allowed_folder_id = "00000000-0000-0000-0000-000000000013"
other_folder_id = "00000000-0000-0000-0000-000000000014"
folder_types = (
("KNOWLEDGE", KnowledgeFolder),
("APPLICATION", ApplicationFolder),
("TOOL", ToolFolder),
)

def setUp(self):
self.user = User.objects.create(
id=self.user_id,
username="folder-tree-user",
nick_name="folder tree user",
password="x",
role="USER",
)
group = SystemUserGroup.objects.create(id=self.group_id, name="group B", workspace_id=self.workspace_id)
SystemUserGroupRelation.objects.create(user=self.user, group=group)

for source, folder_model in self.folder_types:
root = folder_model.objects.create(id=self.workspace_id, name="root", workspace_id=self.workspace_id)
folder_model.objects.create(
id=self.allowed_folder_id, name="111", workspace_id=self.workspace_id, parent=root
)
folder_model.objects.create(
id=self.other_folder_id, name="other", workspace_id=self.workspace_id, parent=root
)
WorkspaceUserGroupResourcePermission.objects.create(
workspace_id=self.workspace_id,
user_group=group,
auth_target_type=source,
target=self.allowed_folder_id,
permission_list=["VIEW"],
)

@patch("folders.serializers.folder.is_workspace_manage", return_value=False)
def test_group_granted_folder_appears_in_each_resource_tree(self, _is_workspace_manage):
for source, _folder_model in self.folder_types:
with self.subTest(source=source):
result = FolderTreeSerializer(
data={"workspace_id": self.workspace_id, "source": source}
).get_folder_tree(SimpleNamespace(id=self.user_id), name=None)

root = next(node for node in result if node["id"] == self.workspace_id)
child_ids = [child["id"] for child in root["children"]]
self.assertIn(self.allowed_folder_id, child_ids)
self.assertNotIn(self.other_folder_id, child_ids)

@shaohuzhang1 shaohuzhang1 Nov 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is focused and readable, but there are several robustness issues and optimization opportunities.

Key concerns

  1. Same folder IDs reused for all three folder types

    • allowed_folder_id, other_folder_id, and workspace_id are reused as primary keys in KnowledgeFolder, ApplicationFolder, and ToolFolder.
    • This is usually fine only if these are completely separate tables. If these models share a base table, use multi-table inheritance, or are involved in generic relations, this can cause collisions or mask bugs.
    • More importantly, because the permission is created for every auth_target_type with the same target ID, a bug where the serializer ignores auth_target_type would still pass. The test should verify source/type isolation with distinct folder IDs per source.

  2. Serializer method called without validation

    result = FolderTreeSerializer(
    data={"workspace_id": self.workspace_id, "source": source}
).get_folder_tree(...)

    The serializer is never validated with .is_valid(). If get_folder_tree() depends on validated_data, this test may fail or behave differently from production. Prefer:

    serializer = FolderTreeSerializer(data={...})
serializer.is_valid(raise_exception=True)
result = serializer.get_folder_tree(self.user, name=None)

    If the method is intentionally called directly, add a comment explaining that it does not depend on validated_data.

  3. Using SimpleNamespace(id=self.user_id) instead of the real user

    • Pass self.user instead. This is more realistic and avoids hiding cases where the serializer uses other user attributes.

  4. Raw password and hard-coded role

    password="x",
role="USER",

    Prefer User.objects.create_user(...) or call set_password(). Also verify that "USER" is a valid role value or use the model’s choice constant.

  5. Possible missing workspace fixture

    • The test assumes workspace_id is just a UUID field. If any of these models use a foreign key to a Workspace, the test may fail or be invalid because no Workspace object is created.

  6. Assertions are a bit shallow

    • assertNotIn(self.other_folder_id, child_ids) only checks direct children of the root. It would not catch other_folder_id appearing deeper in the tree.
    • assertIn(...) does not catch duplicate nodes. Consider:
    self.assertEqual(child_ids.count(self.allowed_folder_id), 1)

    and a recursive check that other_folder_id is absent from the entire tree.

  7. Root ID equals workspace ID

    root = folder_model.objects.create(id=self.workspace_id, ...)

    This may be an implementation detail. If the root folder ID is not guaranteed to equal workspace_id, use a separate root_id and assert against that.

Optimization suggestions

  • Use setUpTestData instead of setUp if the suite grows. It runs once per test class and is faster.
  • Use unique IDs per folder type, or create the permission only inside each subTest for the current source.
  • Assert that is_workspace_manage was called if that behavior matters.
  • Consider extracting a helper to create a root/allowed/other folder set to reduce duplication.

Example improvement sketch

@classmethod
def setUpTestData(cls):
    cls.user = User.objects.create_user(
        id=cls.user_id,
        username="folder-tree-user",
        nick_name="folder tree user",
        password="x",
        role="USER",
    )
    cls.group = SystemUserGroup.objects.create(
        id=cls.group_id, name="group B", workspace_id=cls.workspace_id
    )
    SystemUserGroupRelation.objects.create(user=cls.user, group=cls.group)

    cls.folder_ids = {}
    for source, folder_model in cls.folder_types:
        root_id = uuid.uuid4()
        allowed_id = uuid.uuid4()
        other_id = uuid.uuid4()

        root = folder_model.objects.create(
            id=root_id, name="root", workspace_id=cls.workspace_id
        )
        folder_model.objects.create(
            id=allowed_id, name="111", workspace_id=cls.workspace_id, parent=root
        )
        folder_model.objects.create(
            id=other_id, name="other", workspace_id=cls.workspace_id, parent=root
        )
        WorkspaceUserGroupResourcePermission.objects.create(
            workspace_id=cls.workspace_id,
            user_group=cls.group,
            auth_target_type=source,
            target=allowed_id,
            permission_list=["VIEW"],
        )
        cls.folder_ids[source] = {
            "root_id": root_id,
            "allowed_id": allowed_id,
            "other_id": other_id,
        }

And in the test, use self.user and assert recursively that the disallowed folder is absent.

Overall: the test covers the happy path, but it could be made more reliable by avoiding shared IDs across folder types, using the real user object, validating the serializer if appropriate, and strengthening the assertions.

Loading