feat: 知识库/Agent/Skill 读取和管理权限分离 - #858
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88e9efc69c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| database["effective_permission"] = permission.value | ||
| database["can_manage"] = permission == ResourcePermission.MANAGE | ||
| filtered_databases.append(database) |
There was a problem hiding this comment.
When a v2 ACL grants an admin READ but not MANAGE, this branch appends the untouched database record to GET /api/knowledge/databases; get_databases() includes raw connector parameters in both metadata and additional_params, including dify_token and notion_token. The detail-route redactor is never called for this list endpoint, so a read-only admin can retrieve credentials that the new permission split is intended to hide; redact sensitive fields before appending read-only records.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 2efacded. Read-only database list entries now pass through the shared redact_database_secrets() helper before being returned, covering both metadata and additional_params. Added a regression test for credential removal.
| params = database.get("additional_params") | ||
| if not isinstance(params, dict): | ||
| return | ||
| database["additional_params"] = { |
There was a problem hiding this comment.
For a read-only admin requesting GET /databases/{kb_id}, this function sanitizes only additional_params, but KnowledgeBaseManager.get_database_info() also returns the same normalized connector configuration under metadata. Consequently Dify/Notion tokens removed from additional_params remain present in metadata; sanitize both representations or stop returning the duplicate sensitive field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 2efacded. The shared redaction helper now sanitizes both additional_params and the duplicate metadata representation on read-only detail responses.
| def user_can_manage_agent(user: User, agent: Agent) -> bool: | ||
| return user.role in ADMIN_ROLES or agent.created_by == str(user.uid) | ||
| if is_builtin_agent(agent): | ||
| return user.role in ADMIN_ROLES | ||
| return resolve_agent_permission(user, agent) == ResourcePermission.MANAGE |
There was a problem hiding this comment.
On upgrade, existing non-builtin Agents still have the legacy {access_level, ...} shape, which resolve_agent_permission() converts to a read-only scope because its default legacy_permission is READ; therefore an admin other than created_by now fails this guard and cannot update or delete records that every admin could manage before this commit. The identical regression exists in user_can_manage_skill() for legacy Skills, so legacy ACL handling needs to preserve the previous admin-management behavior without granting that behavior to new v2 scopes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 2efacded. Legacy flat Agent/Skill ACLs now preserve the previous admin management behavior, while ordinary users are capped at read for legacy scopes. New v2 scopes still use their explicit manage scope.
| if force_private: | ||
| if not user_uid: | ||
| raise ValueError("私有智能体必须绑定创建用户") | ||
| return {"access_level": "user", "department_ids": [], "user_uids": [str(user_uid)]} | ||
| return {"version": 2, "read_scope": None, "manage_scope": None} |
There was a problem hiding this comment.
When an ordinary user is granted MANAGE on an Agent created by someone else, the new resolver lets them pass the update guard, but AgentRepository.update() still calls this branch for every non-admin updater. Any edit therefore replaces the ACL with owner-only null scopes while leaving created_by unchanged, immediately removing the delegated manager's own access and discarding all existing readers; preserve the submitted ACL for users who obtained management through manage_scope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 2efacded. Agent updates only force owner-only private scopes when the existing Agent is private; delegated managers updating a shared Agent preserve the submitted ACL. Added a regression test.
| if manage_level == "global" or (read_level == "user" and manage_level != "user"): | ||
| raise ValueError("管理范围必须包含在读取范围内") | ||
| if read_level == manage_level == "department": | ||
| if not set(manage_scope["department_ids"]).issubset(read_scope["department_ids"]): | ||
| raise ValueError("管理范围必须包含在读取范围内") | ||
| elif read_level == manage_level == "user": | ||
| if not set(manage_scope["user_uids"]).issubset(read_scope["user_uids"]): | ||
| raise ValueError("管理范围必须包含在读取范围内") |
There was a problem hiding this comment.
When read_scope is department-based and manage_scope selects individual users, this validation accepts every user UID without checking that those users belong to one of the readable departments. The saved configuration then advertises management to out-of-department users, but resolve_resource_permission() requires them to match both scopes and silently returns NONE; validate membership or reject this cross-scope combination so a successfully saved manager actually receives access.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 2efacded. Strict validation now rejects mismatched non-global read/manage access levels, including department-read plus user-manage, instead of saving a configuration that cannot grant the manager access. The frontend validator matches this rule.
| } else { | ||
| updateData.additional_params = { | ||
| chunk_preset_id: editForm.chunk_preset_id || DEFAULT_CHUNK_PRESET_ID | ||
| } |
There was a problem hiding this comment.
For non-connector knowledge bases, the redesigned configuration modal no longer includes or submits auto_generate_questions; new databases are hard-coded to False, and the only remaining frontend references merely read this flag after uploads. As a result users can no longer enable or disable the existing automatic sample-question behavior through the UI, so retain this field in the new basic or retrieval tab and include it in additional_params.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 2efacded. Restored the non-connector auto_generate_questions switch in the Basic Info tab and include its value in additional_params on save.
| dependency_scopes = [scope for scope in (dep_config["read_scope"], dep_config["manage_scope"]) if scope] | ||
| parent_scopes = [scope for scope in (parent_config["read_scope"], parent_config["manage_scope"]) if scope] | ||
| if not dependency_scopes or not parent_scopes: | ||
| return False |
There was a problem hiding this comment.
When a v2 parent Skill has both scopes disabled, it is intentionally owner-only because ownership still grants MANAGE, but parent_scopes is empty and this branch rejects every non-builtin dependency—even a globally readable dependency or an owner-only dependency created by the same user. Account for the implicit owner audience instead of treating an empty explicit scope as having no users.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 2efacded. Owner-only v2 Skills now use the implicit owner UID as their effective audience when explicit scopes are empty, allowing dependency checks against visible global or owner-only Skills.
|
|
||
| ## v0.7.2 (current) | ||
|
|
||
| - 统一 Agent、Skill 与知识库共享权限：配置拆分读取/管理范围并统一解析 `none/read/manage`；知识库读取范围必填、管理范围不得越界且可为空，创建者与超管保留管理权；旧范围复制为读/管两份。知识库配置弹窗按基础信息、权限、检索分栏，保存后保持打开。 |
There was a problem hiding this comment.
This is a large cross-cutting change spanning the shared permission model, multiple backend resource types, routers, tests, and several frontend management flows, but the commit adds no dated document under docs/vibe recording its requirements, acceptance criteria, goal, and checklist. Add that document so the intended migration and read/manage semantics can be reviewed and maintained against the repository's required workflow.
AGENTS.md reference: AGENTS.md:L101-L102
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
更正：docs/vibe 按项目约定不应提交。我已将这些文件从 PR 最终树移除并保留为本地忽略文件；本条 Review 不需要提交设计文档。
| def _normalize_scope(scope: dict | None) -> dict | None: | ||
| if scope is None: |
There was a problem hiding this comment.
The newly introduced _normalize_scope, _value, and _minimum_permission functions have no docstrings, despite the repository requiring every new function or class to have a concise Chinese docstring. Document their normalization, mapping/object lookup, and permission-order behavior rather than leaving these central ACL helpers undocumented.
AGENTS.md reference: AGENTS.md:L73-L73
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 2efacded. Added concise Chinese docstrings to _normalize_scope, _value, and _minimum_permission as required by the project guidelines.
|
本轮 Review 意见已全部处理，修复提交为
验证：权限相关单元测试 |
|
补充优化（提交 |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
- 基本信息 tab 修改名称、描述、图标或共享权限后不再出现「保存（有修改）」： 弹窗内追踪 profile dirty，与 store 的 hasConfigChanges 合并后驱动按钮显示。 保存按钮改用 size=small (24px) 与标题等高，避免撑高 header。 - 新建模式下原条件永远不满足、按钣缺失「创建」入口：v-if 增加 !editingAgentId， 按钮文案根据模式在创建 / 保存之间切换。 - 新建时自动填充默认名称（"新建智能体"）与 slug（agent-时间戳），用户可直接修改； 名称输入自动 focus + select 便于覆盖默认。 - 共享权限切到部门共享或指定人后，因为按钮撑高导致 card 增高； 将 card-action / title / icon 统一为 20px 等高卡位，三种模式下卡片高度保持不变。
There was a problem hiding this comment.
Pull request overview
本 PR 将 Agent / Skill / 知识库的共享权限模型统一为 none/read/manage 语义，并将知识库共享配置拆分为“读取范围 / 管理范围”，同时重构知识库配置 UI（分 TAB、保存不关闭、检索测试布局调整）以匹配新的权限体验与安全约束。
Changes:
- 新增统一权限解析模块（v2 share_config：
read_scope/manage_scope），并在后端路由与序列化中输出effective_permission/can_manage。 - 知识库共享改为读/管分离：管理范围不得越界、可为空；普通用户共享不再获得管理权限；只读用户获取知识库信息时会脱敏连接器凭据。
- 前端知识库配置弹窗改为“基础信息 / 权限配置 / 检索配置”三 TAB；文件列表增加 token/chunk 内容量与创建者展示，并在只读状态下禁用写操作。
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web/src/views/DataBaseView.vue | 知识库创建/编辑共享配置切换到 v2（read/manage scope），并按 can_manage 隐藏编辑/删除入口。 |
| web/src/views/DataBaseInfoView.vue | 知识库详情页配置弹窗拆分 TAB、保存不关闭；只读用户禁用上传/操作并将文件表格设为只读。 |
| web/src/utils/shareConfig.js | 更新共享配置标签生成逻辑以支持 v2 读/管范围展示。 |
| web/src/components/ShareConfigForm.vue | 共享配置组件升级为 read/manage 两段式配置与范围越界校验（含 requireReadScope）。 |
| web/src/components/SearchConfigPanel.vue | 保存接口支持 notify 开关，供弹窗“静默保存”检索配置使用。 |
| web/src/components/ModelSelectorComponent.vue | 调整模型选择下拉样式（输入框与菜单组间距）。 |
| web/src/components/model-management/AgentManagePanel.vue | 统计逻辑兼容 v2 share_config（global 判断基于 read_scope/legacy）。 |
| web/src/components/model-management/AgentEditModal.vue | Agent 编辑弹窗默认 share_config 改为 v2（read_scope/manage_scope）。 |
| web/src/components/FileTable.vue | 新增内容量与创建人列；支持 readonly 模式禁用写操作与多选。 |
| web/src/components/extensions/SkillInstallFlowModal.vue | Skill 安装流程 share_config 升级为 v2 并兼容展示/克隆。 |
| web/src/components/extensions/SkillDetailView.vue | Skill 详情页 share_config 升级为 v2 并兼容克隆。 |
| docs/develop-guides/changelog.md | 记录权限模型统一、知识库配置 UI 调整与文件列表增强等变更。 |
| backend/test/unit/services/test_skill_service.py | Skill service 单测更新：v2 share_config、依赖范围覆盖规则、owner-only 依赖场景。 |
| backend/test/unit/routers/test_knowledge_resource_permission.py | 新增知识库路由所需权限分类、凭据脱敏、只读管理员禁止更新等单测。 |
| backend/test/unit/repositories/test_agent_repository.py | Agent repository 单测更新：默认 share_config v2、委托管理更新保留 ACL。 |
| backend/test/unit/permissions/test_resource_permission.py | 新增权限解析模块单测：v2 校验、legacy 兼容、权限上限与拒绝逻辑。 |
| backend/test/unit/knowledge/test_file_listing_scaling.py | 文件列表返回值扩展：chunk/token/created_by 与批量用户信息补全。 |
| backend/test/integration/api/test_knowledge_router.py | 知识库接口集成测试更新：默认 share_config v2、部门/用户共享断言改为 manage_scope。 |
| backend/server/routers/skill_router.py | Skill 序列化返回新增 effective_permission 字段。 |
| backend/server/routers/knowledge_router.py | 知识库路由引入基于资源 ACL 的权限校验、GET 输出权限字段并对只读脱敏。 |
| backend/package/yuxi/repositories/user_repository.py | 新增 list_by_uids 供文件列表批量补全创建者信息。 |
| backend/package/yuxi/repositories/knowledge_file_repository.py | 文件列表 SQL 选择字段扩展：chunk/token/created_by。 |
| backend/package/yuxi/repositories/agent_repository.py | Agent share_config 迁移到 v2，权限判定改用统一 resolver，并在序列化中输出 effective_permission。 |
| backend/package/yuxi/permissions/resource_permission.py | 新增统一权限解析实现（normalize + resolve + require），覆盖 Agent/Skill/知识库策略差异。 |
| backend/package/yuxi/permissions/init.py | 导出 permissions 公共 API。 |
| backend/package/yuxi/knowledge/manager.py | 知识库 share_config 规范化为 v2、只读脱敏、文件列表补全创建者信息。 |
| backend/package/yuxi/agents/skills/service.py | Skill 权限判定/规范化改用统一 resolver，并更新依赖可见性覆盖逻辑。 |
| backend/package/yuxi/agents/skills/repository.py | 内置 Skill share_config 更新为 v2 结构。 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| share_config = agent.share_config or DEFAULT_SHARE_CONFIG.copy() | ||
| if share_config.get("access_level") != "global": | ||
| read_scope = share_config.get("read_scope") or {} | ||
| if read_scope.get("access_level") != "global": | ||
| raise ValueError("内置智能体必须全局共享") |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7fd862a23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| force_private=bool( | ||
| updater | ||
| and updater.role not in ADMIN_ROLES | ||
| and not current_config["read_scope"] | ||
| and not current_config["manage_scope"] |
There was a problem hiding this comment.
For an ordinary user delegated MANAGE on a shared Agent, this condition disables force_private, while normalize_agent_share_config() accepts arbitrary v2 access levels. A crafted PUT /agent/{id} can therefore replace the ACL with global read/manage scopes and grant every user permission to edit or delete the Agent, even though the frontend restricts ordinary users to user-level scopes; enforce those role limits in the backend or preserve the ACL for non-admin updaters.
AGENTS.md reference: AGENTS.md:L110-L110
Useful? React with 👍 / 👎.
| elif scope_matches(user, config["manage_scope"]) and ( | ||
| config["read_scope"] is None or scope_matches(user, config["read_scope"]) | ||
| ): | ||
| granted = ResourcePermission.MANAGE | ||
| elif scope_matches(user, config["read_scope"]): |
There was a problem hiding this comment.
When a pre-upgrade Agent or non-builtin Skill has a legacy department/user ACL and the current admin is outside that scope, these branches derive NONE before the role ceiling is applied, whereas the previous user_can_manage_agent() and user_can_manage_skill() granted every admin management access. Fresh evidence in the final tree is that legacy compatibility only caps ordinary users and the added regression test covers only a global legacy ACL; add an admin legacy-management path for Agent and Skill without changing explicit v2 scopes.
Useful? React with 👍 / 👎.
| class ShareableResource(Protocol): | ||
| created_by: str | None | ||
| share_config: dict | None |
There was a problem hiding this comment.
The final tree still introduces ShareableResource without a Chinese docstring. Prior feedback addressed three permission helper functions, but this newly added protocol remains undocumented; add a concise Chinese docstring describing the created_by and share_config contract.
AGENTS.md reference: AGENTS.md:L73-L73
Useful? React with 👍 / 👎.
| console.error('表单验证失败:', err) | ||
| }) | ||
| } | ||
| const searchConfigSaved = await searchConfigPanelRef.value?.save({ notify: false }) |
There was a problem hiding this comment.
When an administrator opens this modal only to rename a knowledge base or change its ACL, this unconditional call still persists the retrieval panel. SearchConfigPanel.loadSavedConfig() overlays server values with browser-local search-config-{kbId} data, so an administrator with stale local storage can silently revert another administrator's newer retrieval settings without visiting the retrieval tab; only save this panel after the user changes it, and avoid treating local storage as authoritative shared configuration.
Useful? React with 👍 / 👎.
| if current_user.role not in {"admin", "superadmin"}: | ||
| raise HTTPException(status_code=403, detail="需要管理员权限") |
There was a problem hiding this comment.
When a knowledge-base creator is later demoted from admin to ordinary user, resolve_knowledge_base_permission() still grants that owner MANAGE—as the new owner regression test explicitly expects—but this role check rejects them before the resource ACL is evaluated. Role changes are supported by the user-management API, so the owner can no longer update or delete their knowledge base despite the ownership guarantee; retain the admin-only check for non-resource routes while allowing a matching owner through kb_id routes.
Useful? React with 👍 / 👎.
| async def get_admin_user(request: Request, current_user: User = Depends(get_required_user)) -> User: | ||
| """兼容原有知识库管理员入口，同时按知识库 ACL 校验资源权限。""" | ||
|
|
||
| if current_user.role not in {"admin", "superadmin"}: | ||
| raise HTTPException(status_code=403, detail="需要管理员权限") |
There was a problem hiding this comment.
The new ACL-aware get_admin_user is local to knowledge_router.py; inspecting knowledge_eval_router.py and graph_router.py shows that both still import the global role-only admin dependency. Consequently an admin with only READ or even NONE on a knowledge base can directly upload, generate, run, resume, or delete its evaluation data and query its graph, including dataset-ID-only deletion routes; move the resource check into a shared dependency and require MANAGE for evaluation mutations and READ for read routes.
AGENTS.md reference: AGENTS.md:L110-L110
Useful? React with 👍 / 👎.
变更描述
统一 Agent、Skill 与知识库的细粒度共享权限，并重构知识库配置体验。
none/read/manage。变更类型
测试
实际验证：
76 passeddocker exec api-dev pytest -q /app/test/unit/permissions/test_resource_permission.py /app/test/unit/repositories/test_agent_repository.py /app/test/unit/services/test_skill_service.py /app/test/integration/api/test_knowledge_router.py→102 passed, 1 skippeddocker exec api-dev uv run ruff check ...→ 通过docker exec api-dev uv run ruff format --check ...→ 通过pnpm lint→ 通过pnpm build→ 通过（仅有依赖包已有的 Rolldown 注释及 chunk size 警告）git diff --check→ 通过相关权限选择器验证记录：Issue #840 comment
本轮 Review 修复后的知识库集成测试受本地 Compose 环境阻塞：Milvus 未监听
milvus:19530，导致测试知识库无法创建；API、PostgreSQL 和其他服务正常启动。说明
Closes #840