[shaohuzhang1]

refactor: add knowledge-base tokenize permission bit

[shaohuzhang1]

Based on the diff, there are no obvious syntax errors, but there are several things to verify:

1. bit_index uniqueness

   • Verify bit_index=73 is not already used within Group.KNOWLEDGE / Group.KNOWLEDGE.
   • SHARED_KNOWLEDGE_TOKEN and RESOURCE_KNOWLEDGE_TOKEN both use bit_index=65. This is fine only if bit_index is scoped per (group, sub_group). If bit indexes are global or mixed across groups, this is a collision.
   • Confirm bit_index=65 is not already used inside Group.SYSTEM_SHARED_KNOWLEDGE or Group.SYSTEM_RES_KNOWLEDGE.

2. 64-bit overflow risk

   • bit_index=65 and bit_index=73 exceed 63. If permission bitmasks are stored as 64-bit integers, BIGINT, or JavaScript numbers, these bits may overflow or lose precision.
   • Ensure the storage/serialization layer supports arbitrary-size integers, byte arrays, strings, or a sufficiently large bitmask type.

3. Security / least privilege for KNOWLEDGE_TOKEN

   • KNOWLEDGE_TOKEN is granted to WORKSPACE_MANAGE and USER, with scopes WORKSPACE and WORKSPACE_RESOURCE.
   • Confirm that normal users should be allowed to trigger knowledge tokenization/indexing. If this is an administrative operation, restrict it to ADMIN or management roles.

4. EE / non-EE consistency

   • SHARED_KNOWLEDGE_TOKEN and RESOURCE_KNOWLEDGE_TOKEN use is_ee=is_ee, while KNOWLEDGE_TOKEN does not.
   • Confirm this is intentional and matches the existing pattern for workspace-level permissions.

5. Operate.TOKEN integration

   • Ensure Operate.TOKEN is defined and handled consistently in:
     • permission serializers/deserializers
     • API authorization decorators
     • frontend permission mappings
     • audit logging
     • any permission migration or cache invalidation logic

6. Naming / comments

   • TOKEN is somewhat ambiguous. If it means “tokenization/indexing,” consider TOKENIZE or INDEX_TOKEN, or add a comment.
   • Add a comment for KNOWLEDGE_TOKEN and SHARED_KNOWLEDGE_TOKEN similar to the existing # 资源管理 - 知识库分词索引 comment.

7. resource_permission_group_list consistency

   • KNOWLEDGE_TOKEN includes resource_permission_group_list=[ResourcePermissionConst.KNOWLEDGE_MANAGE].
   • Confirm that shared/resource token permissions intentionally do not need a similar mapping.

Overall: the main risks are bit-index collisions and >64-bit permission storage. The rest are consistency and least-privilege checks.

[shaohuzhang1]

Here's my review of the added test code:

Issues & Potential Problems

1. Incorrect patch signature / argument order (critical)

@patch("common.log.log.Log")
@patch("knowledge.views.knowledge.KnowledgeSerializer.Operate")
def test_...(self, serializer, log):

Decorators apply bottom-up, so the innermost decorator is the last argument. That means serializer would actually receive the Log mock and log would receive Operate. The parameter names map in reverse order to what you likely intend.

• Bottom decorator (Operate) → first arg after self = serializer ✅ actually correct
• Top decorator (Log) → second arg = log ✅

So the order happens to be correct here, but it's fragile. Since you're asserting only on serializer, consider not patching Log at all, or naming parameters explicitly as mock_operate, mock_log for clarity.

2. KnowledgeView.Tokenize() is instantiated fresh each call — verify this is intended

You create a new view instance three times (Tokenize().put(...)). If Tokenize holds any state across calls, or if assert_not_called() relies on the same serializer mock being reset, confirm the mock is shared correctly. You do reset_mock() inside the loop, which is fine, but note that you never reset before the first negative-case assertion — you rely on reset_mock() being called at the top of each iteration. This works but is easy to misread. Suggest resetting once before the loop:

serializer.reset_mock()
for permission in (...):
    ...

3. Missing assertion on the happy path response

On the successful call you only assert serializer.assert_called_once(). Consider also asserting that tokenize was actually invoked with expected args (e.g., serializer.return_value.tokenize.assert_called_once()), otherwise the test doesn't prove the correct method ran.

4. No assertion that the exception type/message is specific

with self.assertRaises(AppUnauthorizedFailed): is good, but consider asserting it raises specifically due to permission checks (e.g., message content), so the test won't pass for the wrong reason.

5. Magic permissions list not parametrized

Using a for loop inside one test means a failure reports the whole test, not which permission failed. Use subTest:

for permission in (...):
    with self.subTest(permission=permission):
        ...

Or split into separate parametrized tests.

6. P.KNOWLEDGE_EDIT grouping is slightly odd

Grouping document-token, image-token, and edit together implies they all fail the tokenize permission check. Make sure that's intentional — if KNOWLEDGE_EDIT should be allowed, this test will silently enforce wrong behavior.

7. Potential missing reset of side effects

serializer.return_value.tokenize.return_value = {} is set once; after reset_mock(), return values persist (reset_mock only clears calls, not return_values by default in older mocks). Fine here, but be aware.

Optimization Suggestions

• Remove the unused Log patch unless required by the code under test to suppress logging.
• Rename params to mock_operate, mock_log for readability.
• Add subTest per permission.
• Add an explicit assertion that tokenize was called (and possibly with expected kwargs).
• Optionally assert the success path returns the expected JSON payload.

Summary

Logic is broadly correct, but the test is brittle due to decorator/argument ordering conventions, silent multi-permission failures, and weak assertions on the positive path. Refactoring lightly will improve reliability and diagnostics.

[shaohuzhang1]

Review of the diff

The change replaces KNOWLEDGE_DOCUMENT_TOKEN with KNOWLEDGE_TOKEN in two permission lookups. Observations:

1. Verify KNOWLEDGE_TOKEN actually exists

   • Confirm that PermissionConstants.KNOWLEDGE_TOKEN is defined and that both helper methods (get_workspace_knowledge_permission() and get_workspace_permission_workspace_manage_role()) are implemented on it. If it was only recently added, ensure no environment/branch has an older constant set.

2. Semantic scope of the permission changed

   • This endpoint class is Tokenize. Moving from a document-token permission to a broader knowledge-token permission widens or shifts the authorization boundary. Double-check this matches the intended access model — e.g., whether users who could tokenize documents before but not general knowledge tokens (or vice versa) will see a behavior change. Add a regression/permission test for the new constant.

3. Consistency across the codebase

   • Grep for other usages of KNOWLEDGE_DOCUMENT_TOKEN. If some remain while this one moved to KNOWLEDGE_TOKEN, you may end up with inconsistent permissions between related endpoints (e.g., upload vs. tokenize). Consider migrating all related call sites in one change.

4. Long line / readability (minor)

   • The lines exceed typical 88–100 chars. Not a bug, but consider wrapping:

     PermissionConstants.KNOWLEDGE_TOKEN.get_workspace_knowledge_permission(),
     PermissionConstants.KNOWLEDGE_TOKEN.get_workspace_permission_workspace_manage_role(),

     either via a local alias or a shorter variable to keep within linters' max-line-length.

5. Duplicated calls (minor optimization)

   • Two method calls on the same constant resolve two permissions; if these getters are non-trivial (DB/lookup based), compute once into locals and reuse. Low priority if they're pure static returns.

6. Missing type ignore comment

   • The decorator still has # type: ignore on the cartesian/extend_schema above; if the type checker flagged the old constant, re-run mypy/pyright to confirm the new name doesn't produce a different diagnostic, and adjust annotations if needed.

7. No behavioral test in the diff

   • Recommend adding/updating a unit test asserting that a user with only KNOWLEDGE_DOCUMENT_TOKEN permission is now denied/allowed as intended, to lock in the authorization change.

Overall: the edit itself is syntactically fine, but verify the new constant’s existence, keep permission definitions consistent across related endpoints, and add a permission test to guard the changed authorization semantics.