-
Notifications
You must be signed in to change notification settings - Fork 248
feat: Add enabled tools flag and environment variable to filter avail… #354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This is definitely the naive approach to this problem. There are many other approaches that could be considered like "toolsets" where groups of tools are enabled/disabled which might look like: uvx mcp-atlassian --toolsets issues,work_logs,boards,sprints Alternatively, as mentioned in the linked issue, dynamic tool discovery, which may provide the best user experience but be more complex. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for implementing this much-needed feature! I've left a few suggestions in the review comments.
Looking at the bigger picture, we should probably plan a more comprehensive refactoring of server.py
in the future, as it's growing quite large. However, this implementation is a great step forward and addresses the immediate need effectively.
The separation of tools into read/write categories and the filtering mechanism are well thought out. Once you address the unit test suggestions, I think this will be ready to merge.
Thanks again for your contribution!
src/mcp_atlassian/server.py
Outdated
@@ -21,12 +21,29 @@ | |||
logger = logging.getLogger("mcp-atlassian") | |||
|
|||
|
|||
def get_enabled_tools() -> list[str] | None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The get_enabled_tools
helper function is a good addition for centralizing the logic to read and parse the ENABLED_TOOLS
environment variable.
Have unit tests been added to cover cases like:
ENABLED_TOOLS
not set (returnsNone
)- Empty string (
ENABLED_TOOLS=""
) - String with only commas/whitespace
- Correct parsing of comma-separated values with/without extra whitespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! As part of the refactoring, I moved the get_enabled_tools
function to utils/tools.py to better organize the code. I've also added comprehensive unit tests that cover all the scenarios you mentioned:
- When
ENABLED_TOOLS
is not set (returns None) - Empty string (
ENABLED_TOOLS=""
) - Strings with only commas and whitespace
- Proper parsing of comma-separated values, including cases with extra whitespace
You can find all these test cases in the test suite. Let me know if you'd like to see any specific test scenarios!
src/mcp_atlassian/server.py
Outdated
return None | ||
|
||
|
||
def should_include_tool(tool_name: str, enabled_tools: list[str] | None) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The should_include_tool
helper function clearly encapsulates the filtering logic.
Similarly, Would you consider adding unit tests verifying its behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Yes, I've actually included unit tests for should_include_tool
in the same test suite. The tests verify:
- When
enabled_tools
is None (includes all tools) - When a tool is in the enabled list
- When a tool is not in the enabled list
These are alongside the get_enabled_tools
tests in tests/unit/utils/test_tools.py
.
# Provide context to the application | ||
yield AppContext(confluence=confluence, jira=jira) | ||
yield AppContext(confluence=confluence, jira=jira, enabled_tools=enabled_tools) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding enabled_tools
to the AppContext
is the correct way to pass this configuration through the application lifecycle. Good integration. 👍
@@ -207,987 +229,999 @@ async def list_tools() -> list[Tool]: | |||
# Add Confluence tools if Confluence is configured | |||
if ctx and ctx.confluence: | |||
# Always add read operations | |||
tools.extend( | |||
[ | |||
confluence_read_tools = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactoring list_tools
to define separate lists for read/write tools per service (confluence_read_tools
, jira_write_tools
, etc.) is a great improvement for clarity and maintainability. The subsequent filtering loops using should_include_tool
are clean and correctly apply the filtering logic after checking the read_only
flag.
+1, it's important to clearly document the exact tool names (e.g., confluence_search
, jira_create_issue
) available for the --enabled-tools
flag or ENABLED_TOOLS
environment variable in the README or other documentation (e.g. .env.example
) for usability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Glad you like the refactoring. I've added documentation in the README that covers both the tool filtering and read/write access control. You'll find examples of specific tool names (like confluence_search
and jira_get_issue
) in the new "Tool Filtering and Access Control" section, along with examples of using both the --enabled-tools
flag and ENABLED_TOOLS
environment variable.
84e14dc
to
585f22a
Compare
585f22a
to
d8f1c92
Compare
- Add documentation for tool filtering using --enabled-tools flag and ENABLED_TOOLS env var - Document read/write access control with READ_ONLY_MODE - Update configuration examples and troubleshooting sections
d8f1c92
to
3190ccf
Compare
@sooperset I ended up having to rebase off main to address some merge conflicts in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rlcurrall Thanks for the excellent implementation! The code is clean, well-tested, and nicely refactored. Moving the helper functions to utils/tools.py
and organizing the tools by category makes the codebase more maintainable.
The documentation is clear and the unit tests are comprehensive. This gives us the flexibility we need for running multiple MCP servers while leaving room for future enhancements.
LGTM! Approving and merging.
Add enabled tools flag and environment variable to filter available tools
Description
This PR adds the ability to filter which tools are available in the MCP Atlassian server through both a command-line flag and an environment variable. This enhancement provides users with fine-grained control over which tools are exposed by the server, allowing for better security and customization of the server's functionality.
Fixes: #259
Changes
--enabled-tools
command-line option to specify which tools to enableENABLED_TOOLS
environment variable support for tool filteringlist_tools()
functionget_enabled_tools()
andshould_include_tool()
for tool filteringTesting
Default behavior:

With filtering applied:

Checklist