feat(backend): add external KB permission resolver extension point#1025
Open
cocowh wants to merge 5 commits intowecode-ai:mainfrom
Open
feat(backend): add external KB permission resolver extension point#1025cocowh wants to merge 5 commits intowecode-ai:mainfrom
cocowh wants to merge 5 commits intowecode-ai:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a pluggable KB permission resolver (lazy-loaded proxy and default impl), integrates it into KB listing and per-user permission checks as a final fallback, updates package exports, and adds tests covering resolver loading and integration. Changes
Sequence DiagramssequenceDiagram
actor User
participant KBS as KnowledgeService
participant DB as Database
participant KPR as kb_permission_resolver
User->>KBS: list_knowledge_bases(ResourceScope.ALL)
KBS->>DB: Query built-in KB IDs (personal/team/org/shared/bound)
DB-->>KBS: Built-in KB IDs
KBS->>KPR: get_accessible_kb_ids(db, user_id)
KPR-->>KBS: External KB IDs (possibly [])
KBS->>DB: Apply combined filters including external IDs
DB-->>KBS: Consolidated KB list
KBS-->>User: Return filtered KBs
sequenceDiagram
actor User
participant KSS as KnowledgeShareService
participant DB as Database
participant KPR as kb_permission_resolver
User->>KSS: get_user_kb_permission(kb_id, user_id)
KSS->>DB: Check if user is creator
alt Creator
DB-->>KSS: Creator -> ADMIN
KSS-->>User: Return ADMIN
else Not Creator
KSS->>DB: Check explicit ResourceMember
alt Explicit member found
DB-->>KSS: Member role
KSS-->>User: Return role
else No explicit member
KSS->>DB: Check group/task bindings
alt Group/task binding grants role
DB-->>KSS: Role
KSS-->>User: Return role
else No built-in grant
KSS->>KPR: resolve(db, kb_id, user_id, kb)
alt External resolver returns role
KPR-->>KSS: Role string
KSS-->>User: Authorize with resolved role
else External resolver returns None
KSS-->>User: Access Denied
end
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/tests/services/readers/test_kb_permission_resolver.py (1)
268-275: Dead assignment:return_valueis immediately overridden byside_effect.Line 268 sets
first.return_value = mock_kb, but line 270 then setsfirst.side_effect = [mock_kb, None]. Sinceside_effecttakes precedence overreturn_value, line 268 has no effect and can be misleading to readers. Consider removing it.♻️ Suggested fix
- # Simulate KB query returning mock_kb - mock_query.return_value.filter.return_value.first.return_value = mock_kb - # Simulate no explicit permission - mock_query.return_value.filter.return_value.first.side_effect = [ + # KB lookup returns mock_kb, subsequent explicit-permission lookup returns None + mock_query.return_value.filter.return_value.first.side_effect = [ mock_kb, # KB lookup None, # no explicit ResourceMember ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/services/readers/test_kb_permission_resolver.py` around lines 268 - 275, Remove the redundant assignment to mock_query.return_value.filter.return_value.first.return_value (the line that sets it to mock_kb) because you immediately override behavior by setting mock_query.return_value.filter.return_value.first.side_effect = [mock_kb, None]; keep only the side_effect assignment so first() returns mock_kb then None, and update any test comments to reflect that ResourceMember lookup is simulated via the side_effect; locate these in the tests in test_kb_permission_resolver.py around the mock_query and mock_resolver setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/tests/services/readers/test_kb_permission_resolver.py`:
- Around line 319-321: The test unpacks three values from
service.get_user_kb_permission into has_access, role, is_creator but never uses
role, causing a Ruff RUF059 warning; change the unpack to use _role (e.g.,
has_access, _role, is_creator) so the unused variable is clearly marked and the
lint warning is silenced—update the unpack in test_kb_permission_resolver where
get_user_kb_permission is called.
- Around line 77-91: The test currently patches kb_permissions._create_resolver
itself, making it tautological; instead remove the patch.object(...) line and
call the real _create_resolver from the imported module to exercise the
implementation: set mock_settings.SERVICE_EXTENSION = "" (as already done),
re-import app.services.readers.kb_permissions, then call
kb_permissions._create_resolver() and assert isinstance(result,
kb_permissions.DefaultKbPermissionResolver) so the test verifies the
no-extension path of _create_resolver rather than a mocked return.
---
Nitpick comments:
In `@backend/tests/services/readers/test_kb_permission_resolver.py`:
- Around line 268-275: Remove the redundant assignment to
mock_query.return_value.filter.return_value.first.return_value (the line that
sets it to mock_kb) because you immediately override behavior by setting
mock_query.return_value.filter.return_value.first.side_effect = [mock_kb, None];
keep only the side_effect assignment so first() returns mock_kb then None, and
update any test comments to reflect that ResourceMember lookup is simulated via
the side_effect; locate these in the tests in test_kb_permission_resolver.py
around the mock_query and mock_resolver setup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1269a40b-01d9-4a32-8c88-ab51964ae0ee
📒 Files selected for processing (6)
backend/app/services/knowledge/knowledge_service.pybackend/app/services/readers/__init__.pybackend/app/services/readers/kb_permissions.pybackend/app/services/share/knowledge_share_service.pybackend/tests/services/readers/__init__.pybackend/tests/services/readers/test_kb_permission_resolver.py
3 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/services/readers/kb_permissions.py (1)
134-148: Silent fallback whenwrap()returns a falsy value.If an extension's
wrap(base)returnsNone(or any falsy value), the loader silently drops it and returnsbasewithout any log, making misconfigured extensions hard to diagnose. Either log a warning or use an explicitis Nonecheck.♻️ Suggested tweak
- ext = importlib.import_module(f"{settings.SERVICE_EXTENSION}.kb_permissions") - result = ext.wrap(base) - if result: - logger.info("KB permission resolver extension loaded") - return result + ext = importlib.import_module(f"{settings.SERVICE_EXTENSION}.kb_permissions") + result = ext.wrap(base) + if result is None: + logger.warning( + "kb_permissions extension wrap() returned None; using default resolver" + ) + else: + logger.info("KB permission resolver extension loaded") + return result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/readers/kb_permissions.py` around lines 134 - 148, The loader currently treats any falsy return from ext.wrap(base) as a success and silently falls back to base; update the logic in the kb_permissions extension-loading block to detect falsy returns from ext.wrap(base) (preferably checking "is None" if None is the intended sentinel) and log a warning (including the extension name via settings.SERVICE_EXTENSION) when ext.wrap returns None or another unexpected falsy value, before returning base; keep the existing ImportError/Exception handlers and only consider the ext.wrap return value for this additional diagnostic logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/knowledge/knowledge_service.py`:
- Around line 423-427: Wrap the external extension call to
kb_permission_resolver.get_accessible_kb_ids in list_knowledge_bases (in
knowledge_service.py) in a try/except so any runtime exception from the
extension is caught, logged via the module logger (include the exception
details), and ext_kb_ids is set to an empty list to allow built-in KB listing to
continue; apply the same defensive pattern around kb_permission_resolver.resolve
calls in knowledge_share_service.py so resolver exceptions degrade gracefully
instead of propagating.
---
Nitpick comments:
In `@backend/app/services/readers/kb_permissions.py`:
- Around line 134-148: The loader currently treats any falsy return from
ext.wrap(base) as a success and silently falls back to base; update the logic in
the kb_permissions extension-loading block to detect falsy returns from
ext.wrap(base) (preferably checking "is None" if None is the intended sentinel)
and log a warning (including the extension name via settings.SERVICE_EXTENSION)
when ext.wrap returns None or another unexpected falsy value, before returning
base; keep the existing ImportError/Exception handlers and only consider the
ext.wrap return value for this additional diagnostic logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0884b28-d214-453a-9161-e96654e8f580
📒 Files selected for processing (5)
backend/app/services/knowledge/knowledge_service.pybackend/app/services/readers/__init__.pybackend/app/services/readers/kb_permissions.pybackend/app/services/share/knowledge_share_service.pybackend/tests/services/readers/test_kb_permission_resolver.py
✅ Files skipped from review due to trivial changes (1)
- backend/app/services/readers/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/services/share/knowledge_share_service.py
3 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/app/services/readers/kb_permissions.py (1)
161-172: Lazy singleton initialization is not thread-safe.
_get()performs a check-then-set onself._instancewithout a lock. Under concurrent first access (e.g., two request threads hittinglist_knowledge_basesandget_user_kb_permissionsimultaneously at startup),_create_resolver()can run more than once — which also re-imports the extension module and re-logs the "extension loaded" message. In practiceimportlib.import_moduleis cached so the extra cost is small, but it's worth athreading.Lock(double-checked locking) or eager initialization at import time to make the "called only once" guarantee that the tests assert actually hold in production.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/readers/kb_permissions.py` around lines 161 - 172, The lazy singleton _LazyReader is not thread-safe: _get performs a check-then-set on _instance without synchronization, so concurrent first accesses can call _create_resolver() multiple times; fix by adding a threading.Lock as a class/member (e.g., _init_lock) and implement double-checked locking inside _get (check _instance, acquire lock, check again, call _create_resolver and assign to _instance) so only one resolver is created, or alternatively perform eager initialization by calling _create_resolver() at module import and initializing _instance once; ensure references in __getattr__ still call _get() and preserve existing symbols (_LazyReader, _get, _instance, _create_resolver, __getattr__).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/services/knowledge/knowledge_service.py`:
- Around line 428-435: The except block in list_knowledge_bases references an
undefined logger causing a NameError if
kb_permission_resolver.get_accessible_kb_ids(db, user_id) raises; add a
module-level logger so the defensive handler works: import logging at the top
and define logger = logging.getLogger(__name__) (so the except can call
logger.warning), or alternatively replace the except's logger usage with a local
logging.getLogger(__name__).warning call; reference:
kb_permission_resolver.get_accessible_kb_ids and the except in
list_knowledge_bases.
---
Nitpick comments:
In `@backend/app/services/readers/kb_permissions.py`:
- Around line 161-172: The lazy singleton _LazyReader is not thread-safe: _get
performs a check-then-set on _instance without synchronization, so concurrent
first accesses can call _create_resolver() multiple times; fix by adding a
threading.Lock as a class/member (e.g., _init_lock) and implement double-checked
locking inside _get (check _instance, acquire lock, check again, call
_create_resolver and assign to _instance) so only one resolver is created, or
alternatively perform eager initialization by calling _create_resolver() at
module import and initializing _instance once; ensure references in __getattr__
still call _get() and preserve existing symbols (_LazyReader, _get, _instance,
_create_resolver, __getattr__).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 23cf7564-f9f8-4149-84e4-4f23f2aa9ce5
📒 Files selected for processing (3)
backend/app/services/knowledge/knowledge_service.pybackend/app/services/readers/kb_permissions.pybackend/tests/services/readers/test_kb_permission_resolver.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/services/readers/test_kb_permission_resolver.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/app/services/readers/kb_permissions.py (1)
135-154: Consider narrowing the fallback error log to includeexc_info.
except Exception as ecorrectly catches the miscellaneous failure modes (missingwrap, wrap raising, etc.), but the warning at line 152 only serializesstr(e), which often loses the traceback for non-ImportErrorfailures originating inside the extension. For an extension-loading seam that is otherwise silent, passingexc_info=Trueon that branch makes diagnosing misbehaving extensions considerably easier without changing behavior.🛠️ Suggested tweak
- except Exception as e: - logger.warning(f"Failed to load kb_permissions extension: {e}") + except Exception as e: # noqa: BLE001 - extension isolation boundary + logger.warning( + "Failed to load kb_permissions extension: %s", e, exc_info=True + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/readers/kb_permissions.py` around lines 135 - 154, The catch-all exception handler that logs "Failed to load kb_permissions extension: {e}" should include the full traceback for easier debugging; in the except Exception block that handles failures from importing or calling settings.SERVICE_EXTENSION.kb_permissions.wrap, change the logger.warning call that currently serializes only str(e) to pass exc_info=True (keeping the same message) so the logger records the exception stack trace when wrap() or module import fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/app/services/readers/kb_permissions.py`:
- Around line 135-154: The catch-all exception handler that logs "Failed to load
kb_permissions extension: {e}" should include the full traceback for easier
debugging; in the except Exception block that handles failures from importing or
calling settings.SERVICE_EXTENSION.kb_permissions.wrap, change the
logger.warning call that currently serializes only str(e) to pass exc_info=True
(keeping the same message) so the logger records the exception stack trace when
wrap() or module import fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5705820f-989c-4623-856b-1fef5456037f
📒 Files selected for processing (2)
backend/app/services/knowledge/knowledge_service.pybackend/app/services/readers/kb_permissions.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/app/services/knowledge/knowledge_service.py (1)
430-437: Silence Ruff BLE001 with an explanatorynoqa.The broad
except Exceptionhere is intentional — the whole point is to prevent a faulty extension from taking down the core listing path — but Ruff will keep flagging it (BLE001). Consider adding a# noqa: BLE001with a short rationale so the intent is explicit and the lint stays clean.✏️ Suggested diff
try: ext_kb_ids = kb_permission_resolver.get_accessible_kb_ids(db, user_id) - except Exception as e: + except Exception as e: # noqa: BLE001 - never let extension break core listing logger.warning( f"kb_permissions extension get_accessible_kb_ids failed: {e}; " "falling back to empty list" ) ext_kb_ids = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/services/knowledge/knowledge_service.py` around lines 430 - 437, Add a targeted `# noqa: BLE001` to the broad except block that wraps the call to kb_permission_resolver.get_accessible_kb_ids so Ruff stops flagging the intentional catch; update the except line in the try/except that assigns ext_kb_ids (the block that logs via logger.warning and falls back to ext_kb_ids = []) to include `# noqa: BLE001` and a short inline rationale (e.g., "intentional broad except to isolate faulty extension"), keeping the rest of the handler (logger.warning and fallback to empty list) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@backend/app/services/knowledge/knowledge_service.py`:
- Around line 430-437: Add a targeted `# noqa: BLE001` to the broad except block
that wraps the call to kb_permission_resolver.get_accessible_kb_ids so Ruff
stops flagging the intentional catch; update the except line in the try/except
that assigns ext_kb_ids (the block that logs via logger.warning and falls back
to ext_kb_ids = []) to include `# noqa: BLE001` and a short inline rationale
(e.g., "intentional broad except to isolate faulty extension"), keeping the rest
of the handler (logger.warning and fallback to empty list) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80c0af4b-55d9-408d-9fc0-134d8959eb0d
📒 Files selected for processing (1)
backend/app/services/knowledge/knowledge_service.py
Introduce IKbPermissionResolver interface and DefaultKbPermissionResolver no-op base in backend/app/services/readers/kb_permissions.py, following the existing groups/group_members reader pattern (ext.wrap(base)). - get_user_kb_permission() calls kb_permission_resolver as the final fallback after all built-in checks — no behaviour change when SERVICE_EXTENSION is unset (default) - list_knowledge_bases() (ALL scope) calls kb_permission_resolver.get_accessible_kb_ids() to include externally-granted KBs in list responses - _LazyReader uses double-checked locking (threading.Lock) to guarantee the singleton is initialised exactly once under concurrent access - Defensive try/except guards both call sites so a faulty extension cannot break core listing or permission checks - Module-level logger added to knowledge_service.py; kb_permission_resolver imported at module level (no circular dependency) - Full test suite: default resolver, extension loading, ImportError/exception fallback, lazy singleton, and integration with get_user_kb_permission
cocowh
force-pushed
the
weagent/kb-permission-extension
branch
from
29a9fa6 to
2db0887
Compare
- Add exc_info=True to exception logging in _create_resolver() - Add noqa: BLE001 comment to document intentional broad exception catch - Improves debugging capability when extension loading fails
Resolved conflicts: - backend/app/services/knowledge/knowledge_service.py (import statements)
4 tasks
…sion systems Add ExtensionTabConfig interface and KbPermissionsPanel component to support custom permission tabs (e.g., ERP department permissions) in the knowledge base permission management UI. Changes: - New KbPermissionsPanel component with extensionTabs prop - Updated KnowledgeDetailPanel to accept permissionExtensionTabs - Exported ExtensionTabConfig type from knowledge module - Added English and Chinese documentation Internal projects can now inject custom permission tabs without modifying core components, following the same pattern as backend IKbPermissionResolver. Refs: PR wecode-ai#1025 backend extension point
cocowh
added a commit
to cocowh/Wegent
that referenced
this pull request
Apr 28, 2026
…sion systems Add ExtensionTabConfig interface and KbPermissionsPanel component to support custom permission tabs (e.g., ERP department permissions) in the knowledge base permission management UI. Changes: - New KbPermissionsPanel component with extensionTabs prop - Updated KnowledgeDetailPanel to accept permissionExtensionTabs - Exported ExtensionTabConfig type from knowledge module - Added English and Chinese documentation Internal projects can now inject custom permission tabs without modifying core components, following the same pattern as backend IKbPermissionResolver. Refs: PR wecode-ai#1025 backend extension point
…sion systems (#27) Add ExtensionTabConfig interface and KbPermissionsPanel component to support custom permission tabs (e.g., ERP department permissions) in the knowledge base permission management UI. Changes: - New KbPermissionsPanel component with extensionTabs prop - Updated KnowledgeDetailPanel to accept permissionExtensionTabs - Exported ExtensionTabConfig type from knowledge module - Added English and Chinese documentation Internal projects can now inject custom permission tabs without modifying core components, following the same pattern as backend IKbPermissionResolver. Refs: PR wecode-ai#1025 backend extension point
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
IKbPermissionResolverinterface andDefaultKbPermissionResolverno-op base inbackend/app/services/readers/kb_permissions.py, following the existinggroups/group_membersreader pattern (ext.wrap(base))get_user_kb_permission()callskbPermissionResolveras the final fallback after all built-in checks — no behaviour change whenSERVICE_EXTENSIONis unset (default)list_knowledge_bases()(ALL scope) callskbPermissionResolver.get_accessible_kb_ids()to include externally-granted KBs in list responsesMotivation
Internal deployments may need to grant KB access based on external systems (e.g. enterprise department membership) without forking core permission logic. This PR introduces the minimal extension points required, keeping all service-specific code isolated in the internal extension package.
Test plan
DefaultKbPermissionResolver.resolvealways returnsNoneDefaultKbPermissionResolver.get_accessible_kb_idsalways returns[]_create_resolverwraps with extension whenSERVICE_EXTENSIONis setSERVICE_EXTENSION=(empty) and confirmkbPermissionResolverreturnsNone/[]ImportErrorlogs a warning and falls back to the default resolver_LazyReaderinitialises only once (lazy singleton)get_user_kb_permissioncalls the external resolver as the last resortSummary by CodeRabbit
New Features
Behavior
Documentation
Tests