feat: Permission-Based Tool Registry#78
Conversation
Add a centralized permission enforcement layer for tool execution by introducing PermissionAwareRegistry, which wraps the existing ToolRegistry and checks RBAC policies before forwarding execution. Changes: - Add ToolPermissionRequirement struct with min_role and dangerous flag - Add default_tool_permissions() mapping all 9 tools to least-privilege roles: Guest: read_file, list_dir Member: write_file, edit_file, web_search, web_fetch, message Admin: exec, spawn - Implement PermissionAwareRegistry with: - check_permission() enforcing RBAC or fallback defaults - execute() with pre-execution permission check - get_permitted_definitions() filtering tools by user role - get_permitted_tool_names() for role-filtered tool listing - ToolExecutor trait impl for mofa-sdk integration - Audit logging integration for all permission checks - Add ToolError::PermissionDenied variant to error.rs - Wire PermissionAwareRegistry into AgentLoop via set_rbac() - Add with_requirements() for custom permission overrides - Add 20 unit tests covering: Default permission map completeness and correctness Fallback enforcement for Guest/Member/Admin/SuperAdmin RBAC manager integration (allow/deny/unconfigured) Custom requirement overrides Filtered tool listing and definitions Audit log emission on permission checks Permission denied error messages Integration test: execute denied returns error
There was a problem hiding this comment.
Pull request overview
Adds a permission-enforced execution layer for tools by introducing a PermissionAwareRegistry wrapper that consults RBAC (and a default per-tool role map) before allowing tool execution, with optional audit logging.
Changes:
- Introduces
core/src/tools/permissions.rswithPermissionAwareRegistry, default tool permission requirements, and unit tests. - Wires the permission-aware executor into
AgentLoopwhen anRbacManageris configured. - Adds
ToolError::PermissionDeniedand re-exports new permission-related APIs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| core/src/tools/permissions.rs | New permission-aware registry wrapper, default tool permission mapping, audit integration, and tests. |
| core/src/tools/mod.rs | Exposes the new permissions module and re-exports its public types/functions. |
| core/src/lib.rs | Re-exports permission-aware tool registry types/functions at crate root. |
| core/src/error.rs | Adds a new ToolError::PermissionDenied variant for permission failures. |
| core/src/agent/loop_.rs | Adds RBAC/audit fields and switches tool execution to PermissionAwareRegistry when RBAC is set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@Nixxx19 commited new changes |
can you click on resolve and close the suggestion which you have implemented already? thank you! |
|
@Nixxx19 done , also some of the suggestions from it were not that accurate so mightve followed a different route in 1 or two places , still its a hefty one so i wont be surprised if i have to make more changes , thank you |
Nixxx19
left a comment
There was a problem hiding this comment.
Thanks for the work here: the RBAC model, tests, and permission registry structure are solid.
I reviewed this with a runtime focus and found blocking issues before merge.
Blocking Findings
1) RBAC is not wired into the running agent loop
AgentLooponly enforces viaPermissionAwareRegistrywhenset_rbac(...)is set.- I could not find any call to
set_rbac(...)in startup flow, including CLI path. - Result: runtime falls back to
ToolRegistryExecutor(no permission gating).
2) Fine-grained checks are bypassed in default tool wiring
- Default tool registration uses
ReadFileTool::new(),WriteFileTool::new(),EditFileTool::new(),ExecTool::new(). - Path/command checks (
check_path_access,check_command_access) only run when tools are created withwith_rbac(...). - Current behavior applies mostly coarse
"execute"checks and misses path whitelist/blacklist + safe command allowlist enforcement.
3) Fail-open behavior in RBAC manager
- Invalid resource format / unknown category / invalid
min_rolecurrently returnAllowed. - For security boundaries this should be fail-closed (or at least explicitly configurable with strict default).
|
thanks for the review , was out for most of the day , will look at it ! |
Fixes #65
Summary
This PR extends the
ToolRegistryto integrate with the existing RBAC system, ensuring tools require appropriate permissions before they can be executed by agents or users. This addresses the security gap where dangerous tools were previously accessible without restriction.Changes Made
ToolMetadatastruct withrequired_permissionanddangerous_operationsfields.ToolRegistry::executeand related methods to perform permission checks against the user's role before executing the tool.filesystem,web,shell,spawn) to return their specific minimumPermissionLevelrequirements.Related
core/src/permissions.rsChecklist
ToolMetadatawith permission requirementsToolRegistrywith permission checking