Skip to content

delete: knowledge export skills on the backend #7721

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
peslyn merged 1 commit into from
Nov 6, 2026
Merged

delete: knowledge export skills on the backend #7721

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
45 changes: 0 additions & 45 deletions apps/common/auth/constants/permission_constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,21 +700,6 @@ class PermissionConstants(Enum):
),
)

KNOWLEDGE_EXPORT_SKILLS = (
Permission(
group=Group.KNOWLEDGE,
sub_group=Group.KNOWLEDGE,
operate=Operate.EXPORT_SKILLS,
bit_index=13,
),
PermissionMeta(
role_list=[RoleConstants.WORKSPACE_MANAGE, RoleConstants.USER],
category=Category.WORKSPACE,
scope=[PermissionScopeConstants.WORKSPACE, PermissionScopeConstants.WORKSPACE_RESOURCE],
resource_permission_group_list=[ResourcePermissionConst.KNOWLEDGE_MANAGE],
),
)

KNOWLEDGE_IMAGE_READ = (
Permission(group=Group.KNOWLEDGE, sub_group=Group.IMAGE, operate=Operate.READ, bit_index=14),
PermissionMeta(
Expand Down Expand Up @@ -2536,21 +2521,6 @@ class PermissionConstants(Enum):
),
)

SHARED_KNOWLEDGE_EXPORT_SKILLS = (
Permission(
group=Group.SYSTEM_SHARED_KNOWLEDGE,
sub_group=Group.SYSTEM_SHARED_KNOWLEDGE,
operate=Operate.EXPORT_SKILLS,
bit_index=64,
),
PermissionMeta(
role_list=[RoleConstants.ADMIN],
category=Category.SHARED,
is_ee=is_ee,
scope=[PermissionScopeConstants.SYSTEM],
),
)

# 共享知识库工作流
SHARED_KNOWLEDGE_WORKFLOW_READ = (
Permission(
Expand Down Expand Up @@ -4416,21 +4386,6 @@ class PermissionConstants(Enum):
),
)

RESOURCE_KNOWLEDGE_EXPORT_SKILLS = (
Permission(
group=Group.SYSTEM_RES_KNOWLEDGE,
sub_group=Group.SYSTEM_RES_KNOWLEDGE,
operate=Operate.EXPORT_SKILLS,
bit_index=45,
),
PermissionMeta(
role_list=[RoleConstants.ADMIN],
category=Category.RESOURCE,
is_ee=is_ee,
scope=[PermissionScopeConstants.SYSTEM],
),
)

RESOURCE_KNOWLEDGE_IMAGE_READ = (
Permission(group=Group.SYSTEM_RES_KNOWLEDGE, sub_group=Group.SYSTEM_IMAGE, operate=Operate.READ, bit_index=46),
PermissionMeta(

@shaohuzhang1 shaohuzhang1 Nov 6, 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.

Based on the provided diff, the changes are syntactically plausible: three PermissionConstants enum members are removed.

However, I see several potential risks and missing follow-ups:

1. High: Dangling references may break startup or authorization

Search the entire codebase for these removed symbols:

  • KNOWLEDGE_EXPORT_SKILLS
  • SHARED_KNOWLEDGE_EXPORT_SKILLS
  • RESOURCE_KNOWLEDGE_EXPORT_SKILLS
  • Operate.EXPORT_SKILLS

If any decorator, authorization check, serializer, fixture, test, or config still references them, it will likely fail with AttributeError, KeyError, or import errors.

2. High: Persistent permissions and role grants need migration

If these permissions are stored in DB tables, role-permission mappings, cached bitmasks, or serialized configs, simply deleting the enum members is not enough. You likely need a data migration to:

  • Remove or disable the permission records.
  • Remove role mappings.
  • Recalculate role bitmasks.
  • Invalidate permission caches.
  • Handle existing records that reference the deleted permission names.

Otherwise, stale grants may remain.

3. High: Do not reuse deleted bit_index values

The removed permissions use:

  • bit_index=13
  • bit_index=64
  • bit_index=45

If those bit indices are later reused for new permissions, existing role bitmasks may unintentionally grant the new permission. This is a privilege-escalation risk. Retire these bit indices permanently, or implement an explicit migration before reuse.

4. High: Check for authorization regressions

If the corresponding export-skill endpoints still exist, removing their permission constants may leave those endpoints:

  • Unprotected,
  • Protected by a different permission unintentionally,
  • Returning 500 instead of 403,
  • Inconsistent between workspace/shared/resource scopes.

Confirm that removing these permissions is intentional and that the endpoints are either removed, feature-disabled, or still correctly authorized.

5. Medium: Breaking change for external consumers

Permission names are often part of API responses, configs, or client behavior. Removing them may be a breaking change. If they were released, consider deprecation/aliasing first unless this is coordinated with all consumers.

6. Medium: Tests and snapshots need updates

Check for tests that assert:

  • The full permission list,
  • Permission counts,
  • Role bitmasks,
  • OpenAPI/schema snapshots,
  • EE permission matrix behavior.

The is_ee=is_ee field on SHARED_KNOWLEDGE_EXPORT_SKILLS suggests EE-specific behavior may be affected.

7. Low: Dead enum values and bit gaps

After removal, Operate.EXPORT_SKILLS may become unused. Also, permission bits are now non-contiguous. That is usually fine, but verify no code assumes contiguous bit ranges or maximum bit indices.

Recommendation

Before merging:

  1. Grep for all removed permission names and Operate.EXPORT_SKILLS.
  2. Add/verify DB migration and cache invalidation.
  3. Confirm endpoints are removed or still properly authorized.
  4. Update tests/snapshots/docs.
  5. Ensure deleted bit_index values are never reused without migration.

The diff itself does not show an obvious syntax error, but without the full repository I cannot confirm that all references and persisted data are handled.

Expand Down
1 change: 0 additions & 1 deletion apps/knowledge/tests/test_permissions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,6 @@ def test_new_permissions_appear_in_workspace_role_tree(self):
self.assertEqual(len(children[group]["permission"]), count)
ids = {p["id"] for p in children["KNOWLEDGE"]["permission"]}
self.assertIn(str(P.KNOWLEDGE_MCP_CONFIG.value), ids)
self.assertIn(str(P.KNOWLEDGE_EXPORT_SKILLS.value), ids)

@patch("knowledge.views.document.DocumentSerializers.Query")
def test_image_and_document_lists_require_their_own_read_permission(self, serializer):

@shaohuzhang1 shaohuzhang1 Nov 6, 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.

Review of the diff

@@ -91,7 +91,6 @@ def test_new_permissions_appear_in_workspace_role_tree(self):
         ids = {p["id"] for p in children["KNOWLEDGE"]["permission"]}
         self.assertIn(str(P.KNOWLEDGE_MCP_CONFIG.value), ids)
-        self.assertIn(str(P.KNOWLEDGE_EXPORT_SKILLS.value), ids)

Issues / irregularities

  1. Test intent no longer matches its name.
    test_new_permissions_appear_in_workspace_role_tree is supposed to assert that new permissions appear. Deleting the assertion silently reduces coverage and lets a regression (permission disappearing from the role tree) pass. Prefer an explicit assertNotIn if removal is intentional, so the expected behavior is documented:

    self.assertNotIn(str(P.KNOWLEDGE_EXPORT_SKILLS.value), ids)

  2. Possible count mismatch.
    The test uses count to validate the number of permissions per group (assertEqual(len(children[group]["permission"]), count)). If KNOWLEDGE_EXPORT_SKILLS was removed from the tree, the expected count likely needs to be decremented; otherwise the test may fail for an unrelated reason, or mask an off-by-one.

  3. Dead code / orphaned enum.
    Check whether P.KNOWLEDGE_EXPORT_SKILLS is still referenced anywhere. If not, remove the enum member (and any serializers/views/migrations that reference it). Leaving it behind is dead code and can confuse future permission-tree work.

  4. Data migration.
    If the permission was removed, ensure a migration cleans up existing Role/permission rows referencing KNOWLEDGE_EXPORT_SKILLS. Otherwise stale DB records may remain and the role tree can behave inconsistently across environments.

  5. No replacement assertion.
    If EXPORT_SKILLS was replaced by a new permission (e.g. renamed), add an assertIn for the new permission so the test still verifies the "new permissions appear" contract.

Minor / optimization suggestions

  • Use assertSet-style comparison or self.assertTrue(expected.issubset(ids)) for clearer intent when checking multiple permissions, or loop with subTest to get per-permission failure messages.
  • The @patch(...) decorator injects serializer, which appears unused in this test. If it's not used, remove it (or assert it was used) to avoid misleading test setup.
  • Given the current date and Python versions, consider assertIn over set membership is fine, but building ids once and using expected - ids in an error message would make failures easier to debug: 
    missing = expected - ids
self.assertFalse(missing, f"Missing permissions: {missing}")
  • Verify the hunk line counts are right (-91,7 +91,6) — they are consistent with a single-line deletion.

Bottom line

The change is syntactically fine, but functionally it weakens the test without documenting the new expected behavior. Replace the deletion with an explicit negative assertion (or add the replacement permission), reconcile count, and clean up any now-unused enum/migration references.

Expand Down
Loading