Feat/tool permission#8617
Conversation
dosubot
Bot
added
size:L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The permission filtering logic for
require_adminis now implemented in multiple places (_filter_tools_by_role,_build_handoff_toolset, and the tool loop runner); consider extracting a shared helper or central check to avoid divergence of behavior over time. - In
ToolTable.vue,getPermissionColorandgetPermissionLabelacceptpermission: stringeven though only'admin' | 'everyone'are valid; tightening the type and reusing therequire_adminboolean directly would improve type-safety and reduce conversion logic. - In the Web UI handler
handleUpdateToolPermission,tool.require_adminis optimistically mutated before the request; to avoid intermediate inconsistent state in the UI, consider cloning/updating local state after a successful response instead of mutating the original object preemptively.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The permission filtering logic for `require_admin` is now implemented in multiple places (`_filter_tools_by_role`, `_build_handoff_toolset`, and the tool loop runner); consider extracting a shared helper or central check to avoid divergence of behavior over time.
- In `ToolTable.vue`, `getPermissionColor` and `getPermissionLabel` accept `permission: string` even though only `'admin' | 'everyone'` are valid; tightening the type and reusing the `require_admin` boolean directly would improve type-safety and reduce conversion logic.
- In the Web UI handler `handleUpdateToolPermission`, `tool.require_admin` is optimistically mutated before the request; to avoid intermediate inconsistent state in the UI, consider cloning/updating local state after a successful response instead of mutating the original object preemptively.
## Individual Comments
### Comment 1
<location path="astrbot/dashboard/routes/tools.py" line_range="579" />
<code_context>
+ .__dict__
+ )
+
+ ok = self.tool_mgr.set_tool_require_admin(tool_name, bool(require_admin))
+ if ok:
+ return Response().ok(None, "Permission updated.").__dict__
</code_context>
<issue_to_address>
**🚨 issue (security):** Avoid using bool() coercion on `require_admin` to prevent accepting truthy strings
`bool(require_admin)` will treat any non-empty string (including `'false'` or `'0'`) as `True`, which can incorrectly grant or revoke admin permissions if a client sends a string. Instead, validate the type (e.g., `isinstance(require_admin, bool)`) or explicitly parse allowed values and reject anything else, rather than coercing.
</issue_to_address>
### Comment 2
<location path="astrbot/core/agent/runners/tool_loop_agent_runner.py" line_range="1063-1071" />
<code_context>
+ getattr(_caller_event, "role", "?"),
+ sender_id,
+ )
+ _append_tool_call_result(
+ func_tool_id,
+ f"error: Permission denied. "
+ f"Tool '{func_tool_name}' requires admin privileges. "
+ f"Your user ID is {sender_id}. "
+ "Ask the AstrBot admin to grant access via "
+ "WebUI → 管理行为 → 函数工具 → 权限列.",
+ )
+ continue
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Reconsider exposing sender_id and hard-coded UI path in tool error message
This message currently exposes `sender_id` and a hard-coded Chinese navigation path. Depending on how `sender_id` maps to user data, returning it to the LLM may disclose unnecessary identifiers; it should likely be kept in logs only. Also, the UI path is language- and layout-specific and may not be valid across locales. Consider removing `sender_id` from the user-facing text and replacing the path with a generic, localized description of how to request access.
```suggestion
_append_tool_call_result(
func_tool_id,
"error: Permission denied. "
f"Tool '{func_tool_name}' requires admin privileges. "
"Please ask your AstrBot administrator to grant you "
"access to this tool through the appropriate "
"management interface.",
)
continue
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| .__dict__ | ||
| ) | ||
|
|
||
| ok = self.tool_mgr.set_tool_require_admin(tool_name, bool(require_admin)) |
There was a problem hiding this comment.
🚨 issue (security): Avoid using bool() coercion on require_admin to prevent accepting truthy strings
bool(require_admin) will treat any non-empty string (including 'false' or '0') as True, which can incorrectly grant or revoke admin permissions if a client sends a string. Instead, validate the type (e.g., isinstance(require_admin, bool)) or explicitly parse allowed values and reject anything else, rather than coercing.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a permission management system for function tools, allowing specific tools to be restricted to admin users only. It includes backend enforcement to block execution and hide restricted tools from non-admin users, persistence of these settings, and WebUI support for configuring permissions. The review comments identify a critical security vulnerability where restoring permissions from a simple list can silently downgrade tools that default to admin-only in code to everyone-accessible. The reviewer suggests using a dictionary map to preserve code-defined defaults.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a role-based permission system for tools, allowing administrators to restrict specific tools to admin-only access. It includes backend enforcement, persistence of permissions, and a dashboard UI to toggle these settings. The review feedback highlights critical security issues where permissions are bypassed for tools passed directly as 'FunctionTool' instances, and when MCP servers are dynamically started without restoring permissions. Additionally, an improvement is suggested to clean up the persistent storage map when a tool's admin requirement is disabled.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements a role-based permission system for tools, allowing administrators to restrict certain tools to admin-only access. It includes backend logic to hide restricted tools from non-admin users, block unauthorized tool execution, and persist permission settings, as well as frontend updates to manage these permissions via the dashboard. The review feedback identifies potential AttributeError risks when accessing the event object in tool_loop_agent_runner.py and astr_main_agent.py if the event is null, and suggests adding unit tests to cover the new permission management features.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if getattr(tool, "require_admin", False): | ||
| logger.debug( | ||
| "Hiding tool '%s' from non-admin user (sender_id=%s).", | ||
| tool.name, | ||
| event.get_sender_id(), | ||
| ) | ||
| continue |
There was a problem hiding this comment.
If event is None (e.g., in some unit tests or system-initiated runs), calling event.get_sender_id() will raise an AttributeError. We should use defensive programming to handle cases where event might be None.
| if getattr(tool, "require_admin", False): | |
| logger.debug( | |
| "Hiding tool '%s' from non-admin user (sender_id=%s).", | |
| tool.name, | |
| event.get_sender_id(), | |
| ) | |
| continue | |
| if getattr(tool, "require_admin", False): | |
| sender_id = event.get_sender_id() if event else "unknown" | |
| logger.debug( | |
| "Hiding tool '%s' from non-admin user (sender_id=%s).", | |
| tool.name, | |
| sender_id, | |
| ) | |
| continue |
| return True | ||
| return False | ||
|
|
||
| def set_tool_require_admin(self, name: str, require: bool) -> bool: |
There was a problem hiding this comment.
The new tool permission functionality (setting, restoring, and checking require_admin permissions) should be accompanied by corresponding unit tests to ensure correctness and prevent regressions.
References
- New functionality, such as handling attachments, should be accompanied by corresponding unit tests.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
细细阅读了一下pr #8512 ，本pr目前存在同名工具冲突的问题：（ |
1 participant
Related #8511 短期实现
Modifications / 改动点
astrbot/core/agent/tool.py：FunctionTool新增require_admin: bool = False字段，默认值保持向后兼容astrbot/core/provider/func_tool_manager.py：新增set_tool_require_admin()持久化权限配置（存储为 map，仅覆盖用户明确配置的工具）；新增_restore_tool_permissions()恢复权限状态；在init_mcp_clients()末尾和_init_mcp_client()末尾分别调用，覆盖批量初始化和动态启动/重连场景astrbot/core/star/star_manager.py：load()末尾调用_restore_tool_permissions()，确保插件工具注册完成后权限正确恢复（含热重载）astrbot/core/astr_agent_tool_exec.py：_build_handoff_toolset()两个分支均加入require_admin过滤，覆盖 Handoff 子 Agent 路径（该路径绕过主对话的组装流程）astrbot/core/astr_main_agent.py：新增_filter_tools_by_role()，在_plugin_tool_fix之后、AgentRunner.reset()之前过滤非管理员无权调用的工具，LLM 不会感知受限工具的存在astrbot/core/agent/runners/tool_loop_agent_runner.py：_handle_function_tools()中execute()前插入权限二次拦截，作为兜底保障astrbot/dashboard/routes/tools.py：get_tool_list返回require_admin字段；新增POST /tools/set-permission接口，内置工具不可修改dashboard/src/components/extension/componentPanel/components/ToolTable.vue：函数工具列表新增"权限"列，readonly工具显示-，其他工具显示可点击下拉，与指令管理页风格一致dashboard/src/components/extension/componentPanel/index.vue：新增handleUpdateToolPermission，绑定@update-permission事件dashboard/src/components/extension/componentPanel/types.ts：ToolItem新增require_admin?: booleandashboard/src/i18n/locales/zh-CN/features/tool-use.json：新增权限相关翻译 keydashboard/src/i18n/locales/en-US/features/tool-use.json：新增权限相关翻译 keydashboard/src/i18n/locales/ru-RU/features/tool-use.json：新增权限相关翻译 keyScreenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库，或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Introduce role-based tool permissions that restrict certain tools to admin users and expose permission management in the web dashboard.
New Features:
Enhancements: