Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-4048396923 **Bito Automatic Review Skipped – PR Already Merged**Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
kgabryje merged PR #38407: URL: https://github.com/apache/superset/pull/38407 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
codeant-ai-for-open-source[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-4047821803 ## **Sequence Diagram** This PR adds RBAC metadata to MCP tools and enforces permission checks before tool execution. The flow now mirrors Flask AppBuilder permissions, with optional bypass via configuration and explicit denial errors when access is missing. ```mermaid sequenceDiagram participant Developer participant ToolDecorator participant AuthHook participant SecurityManager participant MCPTool Developer->>ToolDecorator: Register tool with class permission and optional method permission ToolDecorator->>AuthHook: Attach permission metadata and wrap tool AuthHook->>AuthHook: Resolve current user and RBAC enabled flag AuthHook->>SecurityManager: Check can access for configured action and resource alt Permission granted or RBAC disabled or no class permission AuthHook->>MCPTool: Execute requested tool MCPTool-->>AuthHook: Return tool result else Permission denied AuthHook-->>Developer: Raise MCPPermissionDeniedError end ``` --- *Generated by [CodeAnt AI](https://codeant.ai)* -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on code in PR #38407:
URL: https://github.com/apache/superset/pull/38407#discussion_r2911816698
##
superset/mcp_service/auth.py:
##
@@ -42,6 +43,90 @@
logger = logging.getLogger(__name__)
+# Constants for RBAC permission attributes (mirrors FAB conventions)
+PERMISSION_PREFIX = "can_"
+CLASS_PERMISSION_ATTR = "_class_permission_name"
+METHOD_PERMISSION_ATTR = "_method_permission_name"
+
+
+class MCPPermissionDeniedError(Exception):
+"""Raised when user lacks required RBAC permission for an MCP tool."""
+
+def __init__(
+self,
+permission_name: str,
+view_name: str,
+user: str | None = None,
+tool_name: str | None = None,
+):
+self.permission_name = permission_name
+self.view_name = view_name
+self.user = user
+self.tool_name = tool_name
+message = (
+f"Permission denied: {permission_name} on {view_name}"
++ (f" for user {user}" if user else "")
++ (f" (tool: {tool_name})" if tool_name else "")
+)
+super().__init__(message)
+
+
+def check_tool_permission(func: Callable[..., Any]) -> bool:
+"""Check if the current user has RBAC permission for an MCP tool.
+
+Reads permission metadata stored on the function by the @tool decorator
+and uses Superset's security_manager to verify access.
+
+Controlled by the ``MCP_RBAC_ENABLED`` config flag (default True).
+Set to False in superset_config.py to disable RBAC checking.
+
+Args:
+func: The tool function with optional permission attributes.
+
+Returns:
+True if user has permission or no permission is required.
+"""
+try:
+from flask import current_app
+
+if not current_app.config.get("MCP_RBAC_ENABLED", True):
+return True
+
+from superset import security_manager
+
+if not hasattr(g, "user") or not g.user:
+logger.warning(
+"No user context for permission check on tool: %s",
func.__name__
+)
+return False
+
+class_permission_name = getattr(func, CLASS_PERMISSION_ATTR, None)
+if not class_permission_name:
+# No RBAC configured for this tool; allow by default.
+return True
+
+method_permission_name = getattr(func, METHOD_PERMISSION_ATTR, "read")
+permission_str = f"{PERMISSION_PREFIX}{method_permission_name}"
+
+has_permission = security_manager.can_access(
+permission_str, class_permission_name
+)
+
+if not has_permission:
+logger.warning(
+"Permission denied for user %s: %s on %s (tool: %s)",
+g.user.username,
+permission_str,
+class_permission_name,
+func.__name__,
+)
+
+return has_permission
+
+except (AttributeError, ValueError, RuntimeError) as e:
Review Comment:
Yes, narrowing the exception to (AttributeError, ValueError, RuntimeError)
improves the code — it handles specific errors from
security_manager.can_access() while letting infrastructure issues like database
errors propagate, avoiding silent access denial.
**superset/mcp_service/auth.py**
```
except (AttributeError, ValueError, RuntimeError) as e:
logger.warning("Error checking tool permission: %s", e)
return False
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
aminghadersohi commented on code in PR #38407:
URL: https://github.com/apache/superset/pull/38407#discussion_r2911807853
##
superset/mcp_service/auth.py:
##
@@ -42,6 +43,90 @@
logger = logging.getLogger(__name__)
+# Constants for RBAC permission attributes (mirrors FAB conventions)
+PERMISSION_PREFIX = "can_"
+CLASS_PERMISSION_ATTR = "_class_permission_name"
+METHOD_PERMISSION_ATTR = "_method_permission_name"
+
+
+class MCPPermissionDeniedError(Exception):
+"""Raised when user lacks required RBAC permission for an MCP tool."""
+
+def __init__(
+self,
+permission_name: str,
+view_name: str,
+user: str | None = None,
+tool_name: str | None = None,
+):
+self.permission_name = permission_name
+self.view_name = view_name
+self.user = user
+self.tool_name = tool_name
+message = (
+f"Permission denied: {permission_name} on {view_name}"
++ (f" for user {user}" if user else "")
++ (f" (tool: {tool_name})" if tool_name else "")
+)
+super().__init__(message)
+
+
+def check_tool_permission(func: Callable[..., Any]) -> bool:
+"""Check if the current user has RBAC permission for an MCP tool.
+
+Reads permission metadata stored on the function by the @tool decorator
+and uses Superset's security_manager to verify access.
+
+Controlled by the ``MCP_RBAC_ENABLED`` config flag (default True).
+Set to False in superset_config.py to disable RBAC checking.
+
+Args:
+func: The tool function with optional permission attributes.
+
+Returns:
+True if user has permission or no permission is required.
+"""
+try:
+from flask import current_app
+
+if not current_app.config.get("MCP_RBAC_ENABLED", True):
+return True
+
+from superset import security_manager
+
+if not hasattr(g, "user") or not g.user:
+logger.warning(
+"No user context for permission check on tool: %s",
func.__name__
+)
+return False
+
+class_permission_name = getattr(func, CLASS_PERMISSION_ATTR, None)
+if not class_permission_name:
+# No RBAC configured for this tool; allow by default.
+return True
+
+method_permission_name = getattr(func, METHOD_PERMISSION_ATTR, "read")
+permission_str = f"{PERMISSION_PREFIX}{method_permission_name}"
+
+has_permission = security_manager.can_access(
+permission_str, class_permission_name
+)
+
+if not has_permission:
+logger.warning(
+"Permission denied for user %s: %s on %s (tool: %s)",
+g.user.username,
+permission_str,
+class_permission_name,
+func.__name__,
+)
+
+return has_permission
+
+except (AttributeError, ValueError, RuntimeError) as e:
Review Comment:
The narrowed exception set `(AttributeError, ValueError, RuntimeError)`
covers the actual failure modes of `security_manager.can_access()`:
AttributeError for missing attributes, ValueError for invalid permission
strings, RuntimeError for context issues. Database errors from SQLAlchemy would
indicate a genuine infrastructure problem that should propagate up rather than
silently returning False (which would deny access to everyone).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-4030824610 AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). Bito Usage Guide **Commands** Type the following command in the pull request comment and save the comment. - `/review` - Manually triggers a full AI review. - `/pause` - Pauses automatic reviews on this pull request. - `/resume` - Resumes automatic reviews. - `/resolve` - Marks all Bito-posted review comments as resolved. - `/abort` - Cancels all in-progress reviews. Refer to the https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands"; target="_blank" rel="noopener noreferrer">documentation for additional commands. **Configuration** This repository uses `Superset` You can customize the agent settings https://alpha.bito.ai/home/ai-agents/code-review-agent"; target="_blank" rel="noopener noreferrer">here or contact your Bito workspace admin at [email protected]. **Documentation & Help** - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance"; target="_blank" rel="noopener noreferrer">Customize agent settings - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules"; target="_blank" rel="noopener noreferrer">Review rules - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent"; target="_blank" rel="noopener noreferrer">General documentation - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs"; target="_blank" rel="noopener noreferrer">FAQ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
codeant-ai-for-open-source[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-4030145236 ## **Sequence Diagram** This PR adds RBAC metadata to MCP tools and enforces Flask-AppBuilder-style permission checks via the MCP auth hook before any tool runs, returning an explicit permission error when access is denied. ```mermaid sequenceDiagram participant ToolDecorator participant ToolFunction as MCP Tool participant Client participant MCPServer as MCP Server participant AuthHook as Auth Hook participant RBAC as Security Manager ToolDecorator->>ToolFunction: Attach class and method permission metadata Client->>MCPServer: Invoke MCP tool MCPServer->>AuthHook: Call tool through auth hook AuthHook->>AuthHook: Load user and read tool permission metadata AuthHook->>RBAC: Check can_action on resource alt Permission granted RBAC-->>AuthHook: Access allowed AuthHook->>ToolFunction: Execute tool ToolFunction-->>Client: Return tool result else Permission denied RBAC-->>AuthHook: Access denied AuthHook-->>Client: MCPPermissionDeniedError end ``` --- *Generated by [CodeAnt AI](https://codeant.ai)* -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-4027666828 AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). Bito Usage Guide **Commands** Type the following command in the pull request comment and save the comment. - `/review` - Manually triggers a full AI review. - `/pause` - Pauses automatic reviews on this pull request. - `/resume` - Resumes automatic reviews. - `/resolve` - Marks all Bito-posted review comments as resolved. - `/abort` - Cancels all in-progress reviews. Refer to the https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands"; target="_blank" rel="noopener noreferrer">documentation for additional commands. **Configuration** This repository uses `Superset` You can customize the agent settings https://alpha.bito.ai/home/ai-agents/code-review-agent"; target="_blank" rel="noopener noreferrer">here or contact your Bito workspace admin at [email protected]. **Documentation & Help** - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance"; target="_blank" rel="noopener noreferrer">Customize agent settings - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules"; target="_blank" rel="noopener noreferrer">Review rules - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent"; target="_blank" rel="noopener noreferrer">General documentation - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs"; target="_blank" rel="noopener noreferrer">FAQ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on code in PR #38407:
URL: https://github.com/apache/superset/pull/38407#discussion_r2908283856
##
superset/mcp_service/auth.py:
##
@@ -42,6 +43,90 @@
logger = logging.getLogger(__name__)
+# Constants for RBAC permission attributes (mirrors FAB conventions)
+PERMISSION_PREFIX = "can_"
+CLASS_PERMISSION_ATTR = "_class_permission_name"
+METHOD_PERMISSION_ATTR = "_method_permission_name"
+
+
+class MCPPermissionDeniedError(Exception):
+"""Raised when user lacks required RBAC permission for an MCP tool."""
+
+def __init__(
+self,
+permission_name: str,
+view_name: str,
+user: str | None = None,
+tool_name: str | None = None,
+):
+self.permission_name = permission_name
+self.view_name = view_name
+self.user = user
+self.tool_name = tool_name
+message = (
+f"Permission denied: {permission_name} on {view_name}"
++ (f" for user {user}" if user else "")
++ (f" (tool: {tool_name})" if tool_name else "")
+)
+super().__init__(message)
+
+
+def check_tool_permission(func: Callable[..., Any]) -> bool:
+"""Check if the current user has RBAC permission for an MCP tool.
+
+Reads permission metadata stored on the function by the @tool decorator
+and uses Superset's security_manager to verify access.
+
+Controlled by the ``MCP_RBAC_ENABLED`` config flag (default True).
+Set to False in superset_config.py to disable RBAC checking.
+
+Args:
+func: The tool function with optional permission attributes.
+
+Returns:
+True if user has permission or no permission is required.
+"""
+try:
+from flask import current_app
+
+if not current_app.config.get("MCP_RBAC_ENABLED", True):
+return True
+
+from superset import security_manager
+
+if not hasattr(g, "user") or not g.user:
+logger.warning(
+"No user context for permission check on tool: %s",
func.__name__
+)
+return False
+
+class_permission_name = getattr(func, CLASS_PERMISSION_ATTR, None)
+if not class_permission_name:
+# No RBAC configured for this tool; allow by default.
+return True
+
+method_permission_name = getattr(func, METHOD_PERMISSION_ATTR, "read")
+permission_str = f"{PERMISSION_PREFIX}{method_permission_name}"
+
+has_permission = security_manager.can_access(
+permission_str, class_permission_name
+)
+
+if not has_permission:
+logger.warning(
+"Permission denied for user %s: %s on %s (tool: %s)",
+g.user.username,
+permission_str,
+class_permission_name,
+func.__name__,
+)
+
+return has_permission
+
+except (AttributeError, ValueError, RuntimeError) as e:
Review Comment:
Exception Handling Too Narrow
Narrowing exception handling to only AttributeError, ValueError, and
RuntimeError may allow other exceptions (e.g., database errors from
security_manager.can_access) to propagate instead of returning False,
potentially crashing MCP tools on permission check failures.
Code suggestion
Check the AI-generated fix before applying
suggestion
except Exception as e:
Code Review Run #0bd4f6
---
Should Bito avoid suggestions like this for future reviews? (https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-4027259924 AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). Bito Usage Guide **Commands** Type the following command in the pull request comment and save the comment. - `/review` - Manually triggers a full AI review. - `/pause` - Pauses automatic reviews on this pull request. - `/resume` - Resumes automatic reviews. - `/resolve` - Marks all Bito-posted review comments as resolved. - `/abort` - Cancels all in-progress reviews. Refer to the https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands"; target="_blank" rel="noopener noreferrer">documentation for additional commands. **Configuration** This repository uses `Superset` You can customize the agent settings https://alpha.bito.ai/home/ai-agents/code-review-agent"; target="_blank" rel="noopener noreferrer">here or contact your Bito workspace admin at [email protected]. **Documentation & Help** - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance"; target="_blank" rel="noopener noreferrer">Customize agent settings - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules"; target="_blank" rel="noopener noreferrer">Review rules - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent"; target="_blank" rel="noopener noreferrer">General documentation - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs"; target="_blank" rel="noopener noreferrer">FAQ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on code in PR #38407: URL: https://github.com/apache/superset/pull/38407#discussion_r2895041389 ## superset/mcp_service/chart/tool/update_chart_preview.py: ## @@ -44,7 +44,7 @@ logger = logging.getLogger(__name__) -@tool(tags=["mutate"]) +@tool(tags=["mutate"], class_permission_name="Chart") Review Comment: This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments. ## tests/unit_tests/mcp_service/test_auth_rbac.py: ## @@ -0,0 +1,221 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for MCP RBAC permission checking (auth.py).""" + +from unittest.mock import MagicMock, patch + +import pytest +from flask import g + +from superset.mcp_service.auth import ( +check_tool_permission, +CLASS_PERMISSION_ATTR, +MCPPermissionDeniedError, +METHOD_PERMISSION_ATTR, +PERMISSION_PREFIX, +) + + [email protected](autouse=True) +def enable_mcp_rbac(app): +"""Re-enable RBAC for dedicated RBAC tests. + +The shared conftest disables RBAC for integration tests. This fixture +overrides that so we can test the actual permission checking logic. +""" +app.config["MCP_RBAC_ENABLED"] = True +yield +app.config.pop("MCP_RBAC_ENABLED", None) + + +class _ToolFunc: +"""Minimal callable stand-in for a tool function in tests. + +Unlike MagicMock, this does NOT auto-create attributes on access, +so ``getattr(func, ATTR, None)`` properly returns None when the +attribute hasn't been set. +""" + +def __init__(self, name: str = "test_tool"): +self.__name__ = name + +def __call__(self, *args, **kwargs): +pass + + +def _make_tool_func( +class_perm: str | None = None, +method_perm: str | None = None, +) -> _ToolFunc: +"""Create a tool function stub with optional permission attributes.""" +func = _ToolFunc() +if class_perm is not None: +setattr(func, CLASS_PERMISSION_ATTR, class_perm) +if method_perm is not None: +setattr(func, METHOD_PERMISSION_ATTR, method_perm) +return func + + +# -- MCPPermissionDeniedError -- + + +def test_permission_denied_error_message_basic(): +err = MCPPermissionDeniedError( +permission_name="can_read", +view_name="Chart", +) +assert "can_read" in str(err) +assert "Chart" in str(err) +assert err.permission_name == "can_read" +assert err.view_name == "Chart" + + +def test_permission_denied_error_message_with_user_and_tool(): +err = MCPPermissionDeniedError( +permission_name="can_write", +view_name="Dashboard", +user="alice", +tool_name="generate_dashboard", +) +assert "alice" in str(err) +assert "generate_dashboard" in str(err) +assert "Dashboard" in str(err) + + +# -- check_tool_permission -- + + +def test_check_tool_permission_no_class_permission_allows(app_context): +"""Tools without class_permission_name should be allowed by default.""" +g.user = MagicMock(username="admin") +func = _make_tool_func() # no class_permission_name +assert check_tool_permission(func) is True + + +def test_check_tool_permission_no_user_denies(app_context): +"""If no g.user, permission check should deny.""" +g.user = None +func = _make_tool_func(class_perm="Chart") +assert check_tool_permission(func) is False + + +def test_check_tool_permission_granted(app_context): +"""When security_manager.can_access returns True, permission is granted.""" +g.user = MagicMock(username="admin") +func = _make_tool_func(class_perm="Chart", method_perm="read") + +with patch("superset.extensions.appbuilder") as mock_appbuilder: +mock_appbuilder.sm.can_access.return_value = True +result = check_tool_permission(func) + +assert result is True +mock_appbuilder.sm.can_access.assert_called_once_with("can_read", "Chart") + + +def test_check_tool_permission_denied(app_context): +"""When security_manager.can_access returns False, permission is denied.""" +g.user = MagicMock(username="viewer") +func = _make_tool_func(class_perm="Dashboard", method_perm="write") + +with patch("sup
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
aminghadersohi commented on code in PR #38407: URL: https://github.com/apache/superset/pull/38407#discussion_r2895039626 ## tests/unit_tests/mcp_service/test_auth_rbac.py: ## @@ -0,0 +1,221 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for MCP RBAC permission checking (auth.py).""" + +from unittest.mock import MagicMock, patch + +import pytest +from flask import g + +from superset.mcp_service.auth import ( +check_tool_permission, +CLASS_PERMISSION_ATTR, +MCPPermissionDeniedError, +METHOD_PERMISSION_ATTR, +PERMISSION_PREFIX, +) + + [email protected](autouse=True) +def enable_mcp_rbac(app): +"""Re-enable RBAC for dedicated RBAC tests. + +The shared conftest disables RBAC for integration tests. This fixture +overrides that so we can test the actual permission checking logic. +""" +app.config["MCP_RBAC_ENABLED"] = True +yield +app.config.pop("MCP_RBAC_ENABLED", None) + + +class _ToolFunc: +"""Minimal callable stand-in for a tool function in tests. + +Unlike MagicMock, this does NOT auto-create attributes on access, +so ``getattr(func, ATTR, None)`` properly returns None when the +attribute hasn't been set. +""" + +def __init__(self, name: str = "test_tool"): +self.__name__ = name + +def __call__(self, *args, **kwargs): +pass + + +def _make_tool_func( +class_perm: str | None = None, +method_perm: str | None = None, +) -> _ToolFunc: +"""Create a tool function stub with optional permission attributes.""" +func = _ToolFunc() +if class_perm is not None: +setattr(func, CLASS_PERMISSION_ATTR, class_perm) +if method_perm is not None: +setattr(func, METHOD_PERMISSION_ATTR, method_perm) +return func + + +# -- MCPPermissionDeniedError -- + + +def test_permission_denied_error_message_basic(): +err = MCPPermissionDeniedError( +permission_name="can_read", +view_name="Chart", +) +assert "can_read" in str(err) +assert "Chart" in str(err) +assert err.permission_name == "can_read" +assert err.view_name == "Chart" + + +def test_permission_denied_error_message_with_user_and_tool(): +err = MCPPermissionDeniedError( +permission_name="can_write", +view_name="Dashboard", +user="alice", +tool_name="generate_dashboard", +) +assert "alice" in str(err) +assert "generate_dashboard" in str(err) +assert "Dashboard" in str(err) + + +# -- check_tool_permission -- + + +def test_check_tool_permission_no_class_permission_allows(app_context): +"""Tools without class_permission_name should be allowed by default.""" +g.user = MagicMock(username="admin") +func = _make_tool_func() # no class_permission_name +assert check_tool_permission(func) is True + + +def test_check_tool_permission_no_user_denies(app_context): +"""If no g.user, permission check should deny.""" +g.user = None +func = _make_tool_func(class_perm="Chart") +assert check_tool_permission(func) is False + + +def test_check_tool_permission_granted(app_context): +"""When security_manager.can_access returns True, permission is granted.""" +g.user = MagicMock(username="admin") +func = _make_tool_func(class_perm="Chart", method_perm="read") + +with patch("superset.extensions.appbuilder") as mock_appbuilder: +mock_appbuilder.sm.can_access.return_value = True +result = check_tool_permission(func) + +assert result is True +mock_appbuilder.sm.can_access.assert_called_once_with("can_read", "Chart") + + +def test_check_tool_permission_denied(app_context): +"""When security_manager.can_access returns False, permission is denied.""" +g.user = MagicMock(username="viewer") +func = _make_tool_func(class_perm="Dashboard", method_perm="write") + +with patch("superset.extensions.appbuilder") as mock_appbuilder: +mock_appbuilder.sm.can_access.return_value = False +result = check_tool_permission(func) + +assert result is False +mock_appbuilder.sm.can_access.assert_called_once_with("can_write", "Dashboard") + + +def test_check_tool_permission_default_method_is_read(app_context): +"""When no metho
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on code in PR #38407:
URL: https://github.com/apache/superset/pull/38407#discussion_r2894954358
##
superset/mcp_service/auth.py:
##
@@ -294,6 +391,17 @@ def sync_wrapper(*args: Any, **kwargs: Any) -> Any:
)
return tool_func(*args, **kwargs)
+# RBAC permission check
+if not check_tool_permission(tool_func):
+raise MCPPermissionDeniedError(
+permission_name=getattr(
+tool_func, METHOD_PERMISSION_ATTR, "read"
+),
Review Comment:
The suggestion correctly fixes the error message to use the full FAB
permission string (e.g., 'can_read') instead of just the method name ('read'),
matching what security_manager.can_access checks. This aligns with the CodeAnt
comments on both async and sync wrappers.
**superset/mcp_service/auth.py**
```
permission_name=f"{PERMISSION_PREFIX}{getattr(tool_func,
METHOD_PERMISSION_ATTR, 'read')}",
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on code in PR #38407: URL: https://github.com/apache/superset/pull/38407#discussion_r2894951338 ## superset/mcp_service/mcp_config.py: ## @@ -45,6 +45,10 @@ # MCP Debug mode - shows suppressed initialization output in stdio mode MCP_DEBUG = False +# MCP RBAC - when True, tools with class_permission_name are checked +# against the FAB security_manager before execution. +MCP_RBAC_ENABLED = True Review Comment: Yes, adding `MCP_RBAC_ENABLED` to the `get_mcp_config` defaults allows users to override the RBAC flag in `superset_config.py`, which aligns with the code's default behavior in `auth.py` and provides configurability. **superset/mcp_service/mcp_config.py** ``` "MCP_DEBUG": MCP_DEBUG, "MCP_RBAC_ENABLED": MCP_RBAC_ENABLED, **MCP_SESSION_CONFIG, **MCP_CSRF_CONFIG, ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on code in PR #38407:
URL: https://github.com/apache/superset/pull/38407#discussion_r2894949893
##
superset/mcp_service/chart/tool/update_chart_preview.py:
##
@@ -44,7 +44,7 @@
logger = logging.getLogger(__name__)
-@tool(tags=["mutate"])
+@tool(tags=["mutate"], class_permission_name="Chart")
Review Comment:
This question isn’t related to the pull request. I can only help with
questions about the PR’s code or comments.
##
superset/mcp_service/auth.py:
##
@@ -42,6 +43,90 @@
logger = logging.getLogger(__name__)
+# Constants for RBAC permission attributes (mirrors FAB conventions)
+PERMISSION_PREFIX = "can_"
+CLASS_PERMISSION_ATTR = "_class_permission_name"
+METHOD_PERMISSION_ATTR = "_method_permission_name"
+
+
+class MCPPermissionDeniedError(Exception):
+"""Raised when user lacks required RBAC permission for an MCP tool."""
+
+def __init__(
+self,
+permission_name: str,
+view_name: str,
+user: str | None = None,
+tool_name: str | None = None,
+):
+self.permission_name = permission_name
+self.view_name = view_name
+self.user = user
+self.tool_name = tool_name
+message = (
+f"Permission denied: {permission_name} on {view_name}"
++ (f" for user {user}" if user else "")
++ (f" (tool: {tool_name})" if tool_name else "")
+)
+super().__init__(message)
+
+
+def check_tool_permission(func: Callable[..., Any]) -> bool:
+"""Check if the current user has RBAC permission for an MCP tool.
+
+Reads permission metadata stored on the function by the @tool decorator
+and uses Superset's security_manager to verify access.
+
+Controlled by the ``MCP_RBAC_ENABLED`` config flag (default True).
+Set to False in superset_config.py to disable RBAC checking.
+
+Args:
+func: The tool function with optional permission attributes.
+
+Returns:
+True if user has permission or no permission is required.
+"""
+try:
+from flask import current_app
+
+if not current_app.config.get("MCP_RBAC_ENABLED", True):
+return True
+
+from superset import security_manager
+
+if not hasattr(g, "user") or not g.user:
+logger.warning(
+"No user context for permission check on tool: %s",
func.__name__
+)
+return False
+
+class_permission_name = getattr(func, CLASS_PERMISSION_ATTR, None)
+if not class_permission_name:
+# No RBAC configured for this tool; allow by default.
+return True
+
+method_permission_name = getattr(func, METHOD_PERMISSION_ATTR, "read")
+permission_str = f"{PERMISSION_PREFIX}{method_permission_name}"
+
+has_permission = security_manager.can_access(
+permission_str, class_permission_name
+)
+
+if not has_permission:
+logger.warning(
+"Permission denied for user %s: %s on %s (tool: %s)",
+g.user.username,
+permission_str,
+class_permission_name,
+func.__name__,
+)
+
+return has_permission
+
+except Exception as e:
Review Comment:
Here's the updated code snippet applying the narrowed exception handling in
the check_tool_permission function:
**superset/mcp_service/auth.py**
```
except (AttributeError, ValueError, RuntimeError) as e:
logger.warning("Error checking tool permission: %s", e)
return False
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on code in PR #38407: URL: https://github.com/apache/superset/pull/38407#discussion_r2894948371 ## tests/unit_tests/mcp_service/test_auth_rbac.py: ## @@ -0,0 +1,221 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for MCP RBAC permission checking (auth.py).""" + +from unittest.mock import MagicMock, patch + +import pytest +from flask import g + +from superset.mcp_service.auth import ( +check_tool_permission, +CLASS_PERMISSION_ATTR, +MCPPermissionDeniedError, +METHOD_PERMISSION_ATTR, +PERMISSION_PREFIX, +) + + [email protected](autouse=True) +def enable_mcp_rbac(app): +"""Re-enable RBAC for dedicated RBAC tests. + +The shared conftest disables RBAC for integration tests. This fixture +overrides that so we can test the actual permission checking logic. +""" +app.config["MCP_RBAC_ENABLED"] = True +yield +app.config.pop("MCP_RBAC_ENABLED", None) + + +class _ToolFunc: +"""Minimal callable stand-in for a tool function in tests. + +Unlike MagicMock, this does NOT auto-create attributes on access, +so ``getattr(func, ATTR, None)`` properly returns None when the +attribute hasn't been set. +""" + +def __init__(self, name: str = "test_tool"): +self.__name__ = name + +def __call__(self, *args, **kwargs): +pass + + +def _make_tool_func( +class_perm: str | None = None, +method_perm: str | None = None, +) -> _ToolFunc: +"""Create a tool function stub with optional permission attributes.""" +func = _ToolFunc() +if class_perm is not None: +setattr(func, CLASS_PERMISSION_ATTR, class_perm) +if method_perm is not None: +setattr(func, METHOD_PERMISSION_ATTR, method_perm) +return func + + +# -- MCPPermissionDeniedError -- + + +def test_permission_denied_error_message_basic(): +err = MCPPermissionDeniedError( +permission_name="can_read", +view_name="Chart", +) +assert "can_read" in str(err) +assert "Chart" in str(err) +assert err.permission_name == "can_read" +assert err.view_name == "Chart" + + +def test_permission_denied_error_message_with_user_and_tool(): +err = MCPPermissionDeniedError( +permission_name="can_write", +view_name="Dashboard", +user="alice", +tool_name="generate_dashboard", +) +assert "alice" in str(err) +assert "generate_dashboard" in str(err) +assert "Dashboard" in str(err) + + +# -- check_tool_permission -- + + +def test_check_tool_permission_no_class_permission_allows(app_context): +"""Tools without class_permission_name should be allowed by default.""" +g.user = MagicMock(username="admin") +func = _make_tool_func() # no class_permission_name +assert check_tool_permission(func) is True + + +def test_check_tool_permission_no_user_denies(app_context): +"""If no g.user, permission check should deny.""" +g.user = None +func = _make_tool_func(class_perm="Chart") +assert check_tool_permission(func) is False + + +def test_check_tool_permission_granted(app_context): +"""When security_manager.can_access returns True, permission is granted.""" +g.user = MagicMock(username="admin") +func = _make_tool_func(class_perm="Chart", method_perm="read") + +with patch("superset.extensions.appbuilder") as mock_appbuilder: +mock_appbuilder.sm.can_access.return_value = True +result = check_tool_permission(func) + +assert result is True +mock_appbuilder.sm.can_access.assert_called_once_with("can_read", "Chart") + + +def test_check_tool_permission_denied(app_context): +"""When security_manager.can_access returns False, permission is denied.""" +g.user = MagicMock(username="viewer") +func = _make_tool_func(class_perm="Dashboard", method_perm="write") + +with patch("superset.extensions.appbuilder") as mock_appbuilder: +mock_appbuilder.sm.can_access.return_value = False +result = check_tool_permission(func) + +assert result is False +mock_appbuilder.sm.can_access.assert_called_once_with("can_write", "Dashboard") + + +def test_check_tool_permission_default_method_is_read(app_context): +"""When n
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
aminghadersohi commented on code in PR #38407: URL: https://github.com/apache/superset/pull/38407#discussion_r2894943661 ## superset/mcp_service/chart/tool/update_chart_preview.py: ## @@ -44,7 +44,7 @@ logger = logging.getLogger(__name__) -@tool(tags=["mutate"]) +@tool(tags=["mutate"], class_permission_name="Chart") Review Comment: Valid point. The `update_chart_preview` tool tagged as `mutate` should indeed use a write permission check rather than defaulting to read. I'll add the appropriate `method_permission_name` in a follow-up. ## tests/unit_tests/mcp_service/test_auth_rbac.py: ## @@ -0,0 +1,221 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for MCP RBAC permission checking (auth.py).""" + +from unittest.mock import MagicMock, patch + +import pytest +from flask import g + +from superset.mcp_service.auth import ( +check_tool_permission, +CLASS_PERMISSION_ATTR, +MCPPermissionDeniedError, +METHOD_PERMISSION_ATTR, +PERMISSION_PREFIX, +) + + [email protected](autouse=True) +def enable_mcp_rbac(app): +"""Re-enable RBAC for dedicated RBAC tests. + +The shared conftest disables RBAC for integration tests. This fixture +overrides that so we can test the actual permission checking logic. +""" +app.config["MCP_RBAC_ENABLED"] = True +yield +app.config.pop("MCP_RBAC_ENABLED", None) + + +class _ToolFunc: +"""Minimal callable stand-in for a tool function in tests. + +Unlike MagicMock, this does NOT auto-create attributes on access, +so ``getattr(func, ATTR, None)`` properly returns None when the +attribute hasn't been set. +""" + +def __init__(self, name: str = "test_tool"): +self.__name__ = name + +def __call__(self, *args, **kwargs): +pass + + +def _make_tool_func( +class_perm: str | None = None, +method_perm: str | None = None, +) -> _ToolFunc: +"""Create a tool function stub with optional permission attributes.""" +func = _ToolFunc() +if class_perm is not None: +setattr(func, CLASS_PERMISSION_ATTR, class_perm) +if method_perm is not None: +setattr(func, METHOD_PERMISSION_ATTR, method_perm) +return func + + +# -- MCPPermissionDeniedError -- + + +def test_permission_denied_error_message_basic(): +err = MCPPermissionDeniedError( +permission_name="can_read", +view_name="Chart", +) +assert "can_read" in str(err) +assert "Chart" in str(err) +assert err.permission_name == "can_read" +assert err.view_name == "Chart" + + +def test_permission_denied_error_message_with_user_and_tool(): +err = MCPPermissionDeniedError( +permission_name="can_write", +view_name="Dashboard", +user="alice", +tool_name="generate_dashboard", +) +assert "alice" in str(err) +assert "generate_dashboard" in str(err) +assert "Dashboard" in str(err) + + +# -- check_tool_permission -- + + +def test_check_tool_permission_no_class_permission_allows(app_context): +"""Tools without class_permission_name should be allowed by default.""" +g.user = MagicMock(username="admin") +func = _make_tool_func() # no class_permission_name +assert check_tool_permission(func) is True + + +def test_check_tool_permission_no_user_denies(app_context): +"""If no g.user, permission check should deny.""" +g.user = None +func = _make_tool_func(class_perm="Chart") +assert check_tool_permission(func) is False + + +def test_check_tool_permission_granted(app_context): +"""When security_manager.can_access returns True, permission is granted.""" +g.user = MagicMock(username="admin") +func = _make_tool_func(class_perm="Chart", method_perm="read") + +with patch("superset.extensions.appbuilder") as mock_appbuilder: +mock_appbuilder.sm.can_access.return_value = True +result = check_tool_permission(func) + +assert result is True +mock_appbuilder.sm.can_access.assert_called_once_with("can_read", "Chart") + + +def test_check_tool_permission_denied(app_context): +"""When security_manager.can_access returns False, permission is denied.""" +g.user = MagicMock(username="viewer") +func = _
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
aminghadersohi commented on code in PR #38407:
URL: https://github.com/apache/superset/pull/38407#discussion_r2894941954
##
tests/unit_tests/mcp_service/test_auth_rbac.py:
##
@@ -0,0 +1,197 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Tests for MCP RBAC permission checking (auth.py)."""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+from flask import g
+
+from superset.mcp_service.auth import (
+check_tool_permission,
+CLASS_PERMISSION_ATTR,
+MCPPermissionDeniedError,
+METHOD_PERMISSION_ATTR,
+PERMISSION_PREFIX,
+)
+
+
+class _ToolFunc:
+"""Minimal callable stand-in for a tool function in tests.
+
+Unlike MagicMock, this does NOT auto-create attributes on access,
+so ``getattr(func, ATTR, None)`` properly returns None when the
+attribute hasn't been set.
+"""
+
+def __init__(self, name: str = "test_tool"):
+self.__name__ = name
+
+def __call__(self, *args, **kwargs):
+pass
+
+
+def _make_tool_func(
+class_perm: str | None = None,
+method_perm: str | None = None,
+) -> _ToolFunc:
+"""Create a tool function stub with optional permission attributes."""
+func = _ToolFunc()
+if class_perm is not None:
+setattr(func, CLASS_PERMISSION_ATTR, class_perm)
+if method_perm is not None:
+setattr(func, METHOD_PERMISSION_ATTR, method_perm)
+return func
+
+
+# -- MCPPermissionDeniedError --
+
+
+def test_permission_denied_error_message_basic():
+err = MCPPermissionDeniedError(
+permission_name="can_read",
+view_name="Chart",
+)
+assert "can_read" in str(err)
+assert "Chart" in str(err)
+assert err.permission_name == "can_read"
+assert err.view_name == "Chart"
+
+
+def test_permission_denied_error_message_with_user_and_tool():
+err = MCPPermissionDeniedError(
+permission_name="can_write",
+view_name="Dashboard",
+user="alice",
+tool_name="generate_dashboard",
+)
+assert "alice" in str(err)
+assert "generate_dashboard" in str(err)
+assert "Dashboard" in str(err)
+
+
+# -- check_tool_permission --
+
+
+def test_check_tool_permission_no_class_permission_allows(app_context):
+"""Tools without class_permission_name should be allowed by default."""
+g.user = MagicMock(username="admin")
+func = _make_tool_func() # no class_permission_name
+assert check_tool_permission(func) is True
+
+
+def test_check_tool_permission_no_user_denies(app_context):
+"""If no g.user, permission check should deny."""
+g.user = None
+func = _make_tool_func(class_perm="Chart")
+assert check_tool_permission(func) is False
+
+
+def test_check_tool_permission_granted(app_context):
+"""When security_manager.can_access returns True, permission is granted."""
+g.user = MagicMock(username="admin")
+func = _make_tool_func(class_perm="Chart", method_perm="read")
+
+with patch("superset.extensions.appbuilder") as mock_appbuilder:
+mock_appbuilder.sm.can_access.return_value = True
+result = check_tool_permission(func)
+
+assert result is True
+mock_appbuilder.sm.can_access.assert_called_once_with("can_read", "Chart")
+
+
+def test_check_tool_permission_denied(app_context):
+"""When security_manager.can_access returns False, permission is denied."""
+g.user = MagicMock(username="viewer")
+func = _make_tool_func(class_perm="Dashboard", method_perm="write")
+
+with patch("superset.extensions.appbuilder") as mock_appbuilder:
+mock_appbuilder.sm.can_access.return_value = False
+result = check_tool_permission(func)
+
+assert result is False
+mock_appbuilder.sm.can_access.assert_called_once_with("can_write",
"Dashboard")
+
+
+def test_check_tool_permission_default_method_is_read(app_context):
+"""When no method_permission_name is set, defaults to 'read'."""
+g.user = MagicMock(username="admin")
+func = _make_tool_func(class_perm="Dataset")
+# No method_perm set - should default to "read"
+
+with patch("superset.extensions.appbuilder") as mock_appbuilder:
+mock_appbuilder.sm.can_access.return_value = True
+check_tool_permission(func)
+
+m
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on code in PR #38407:
URL: https://github.com/apache/superset/pull/38407#discussion_r2892524349
##
superset/mcp_service/auth.py:
##
@@ -294,6 +391,17 @@ def sync_wrapper(*args: Any, **kwargs: Any) -> Any:
)
return tool_func(*args, **kwargs)
+# RBAC permission check
+if not check_tool_permission(tool_func):
+raise MCPPermissionDeniedError(
+permission_name=getattr(
+tool_func, METHOD_PERMISSION_ATTR, "read"
+),
Review Comment:
Incorrect permission name in error message
The exception message incorrectly shows the method permission name rather
than the full FAB permission string checked by security_manager.can_access().
This could confuse debugging, as logs show 'can_read' but errors say 'read'.
Code suggestion
Check the AI-generated fix before applying
suggestion
permission_name=f"{PERMISSION_PREFIX}{getattr(tool_func,
METHOD_PERMISSION_ATTR, 'read')}",
Code Review Run #7c2718
---
Should Bito avoid suggestions like this for future reviews? (https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules)
- [ ] Yes, avoid them
##
superset/mcp_service/auth.py:
##
@@ -42,6 +43,90 @@
logger = logging.getLogger(__name__)
+# Constants for RBAC permission attributes (mirrors FAB conventions)
+PERMISSION_PREFIX = "can_"
+CLASS_PERMISSION_ATTR = "_class_permission_name"
+METHOD_PERMISSION_ATTR = "_method_permission_name"
+
+
+class MCPPermissionDeniedError(Exception):
+"""Raised when user lacks required RBAC permission for an MCP tool."""
+
+def __init__(
+self,
+permission_name: str,
+view_name: str,
+user: str | None = None,
+tool_name: str | None = None,
+):
+self.permission_name = permission_name
+self.view_name = view_name
+self.user = user
+self.tool_name = tool_name
+message = (
+f"Permission denied: {permission_name} on {view_name}"
++ (f" for user {user}" if user else "")
++ (f" (tool: {tool_name})" if tool_name else "")
+)
+super().__init__(message)
+
+
+def check_tool_permission(func: Callable[..., Any]) -> bool:
+"""Check if the current user has RBAC permission for an MCP tool.
+
+Reads permission metadata stored on the function by the @tool decorator
+and uses Superset's security_manager to verify access.
+
+Controlled by the ``MCP_RBAC_ENABLED`` config flag (default True).
+Set to False in superset_config.py to disable RBAC checking.
+
+Args:
+func: The tool function with optional permission attributes.
+
+Returns:
+True if user has permission or no permission is required.
+"""
+try:
+from flask import current_app
+
+if not current_app.config.get("MCP_RBAC_ENABLED", True):
+return True
+
+from superset import security_manager
+
+if not hasattr(g, "user") or not g.user:
+logger.warning(
+"No user context for permission check on tool: %s",
func.__name__
+)
+return False
+
+class_permission_name = getattr(func, CLASS_PERMISSION_ATTR, None)
+if not class_permission_name:
+# No RBAC configured for this tool; allow by default.
+return True
+
+method_permission_name = getattr(func, METHOD_PERMISSION_ATTR, "read")
+permission_str = f"{PERMISSION_PREFIX}{method_permission_name}"
+
+has_permission = security_manager.can_access(
+permission_str, class_permission_name
+)
+
+if not has_permission:
+logger.warning(
+"Permission denied for user %s: %s on %s (tool: %s)",
+g.user.username,
+permission_str,
+class_permission_name,
+func.__name__,
+)
+
+return has_permission
+
+except Exception as e:
Review Comment:
Blind exception catch too broad
Replace the broad `Exception` catch with specific exception types (e.g.,
`AttributeError`, `ValueError`, `RuntimeError`) to avoid masking unexpected
errors.
Code suggestion
Check the AI-generated fix before applying
suggestion
except (AttributeError, ValueError, RuntimeError) as e:
Code Review Run #7c2718
---
Should Bito avoid suggestions like this for future reviews? (https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules)
- [ ] Yes, avoid them
--
This is an automated message from the Apache Git
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-4007887748 AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). Bito Usage Guide **Commands** Type the following command in the pull request comment and save the comment. - `/review` - Manually triggers a full AI review. - `/pause` - Pauses automatic reviews on this pull request. - `/resume` - Resumes automatic reviews. - `/resolve` - Marks all Bito-posted review comments as resolved. - `/abort` - Cancels all in-progress reviews. Refer to the https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands"; target="_blank" rel="noopener noreferrer">documentation for additional commands. **Configuration** This repository uses `Superset` You can customize the agent settings https://alpha.bito.ai/home/ai-agents/code-review-agent"; target="_blank" rel="noopener noreferrer">here or contact your Bito workspace admin at [email protected]. **Documentation & Help** - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance"; target="_blank" rel="noopener noreferrer">Customize agent settings - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules"; target="_blank" rel="noopener noreferrer">Review rules - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent"; target="_blank" rel="noopener noreferrer">General documentation - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs"; target="_blank" rel="noopener noreferrer">FAQ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
codeant-ai-for-open-source[bot] commented on code in PR #38407:
URL: https://github.com/apache/superset/pull/38407#discussion_r2892095233
##
superset/mcp_service/auth.py:
##
@@ -264,6 +350,17 @@ async def async_wrapper(*args: Any, **kwargs: Any) -> Any:
)
return await tool_func(*args, **kwargs)
+# RBAC permission check
+if not check_tool_permission(tool_func):
+raise MCPPermissionDeniedError(
+permission_name=getattr(
+tool_func, METHOD_PERMISSION_ATTR, "read"
+),
+view_name=getattr(tool_func, CLASS_PERMISSION_ATTR,
"unknown"),
+user=user.username,
+tool_name=tool_func.__name__,
+)
+
Review Comment:
**Suggestion:** When RBAC denies access for an async MCP tool, the raised
MCPPermissionDeniedError currently uses the bare action name (e.g. "read" or
"execute_sql_query") instead of the full permission string ("can_read",
"can_execute_sql_query") that was actually checked, which can confuse debugging
and any code/tests that expect the real permission identifier. [logic error]
Severity Level: Major ⚠️
```mdx
- ⚠️ Async MCP RBAC errors expose incorrect permission identifier.
- ⚠️ Debugging permission denials for async tools becomes harder.
```
```suggestion
# RBAC permission check
method_name = getattr(tool_func, METHOD_PERMISSION_ATTR,
"read")
permission_str = f"{PERMISSION_PREFIX}{method_name}"
if not check_tool_permission(tool_func):
raise MCPPermissionDeniedError(
permission_name=permission_str,
view_name=getattr(tool_func,
CLASS_PERMISSION_ATTR, "unknown"),
user=user.username,
tool_name=tool_func.__name__,
)
```
Steps of Reproduction ✅
```mdx
1. Start the MCP server via `superset mcp`, which exposes MCP tools wrapped
by
`mcp_auth_hook` in `superset/mcp_service/auth.py` (async wrapper around
lines 337–375,
RBAC block at 355–363).
2. Authenticate as a Superset user who lacks the `can_read` permission on a
view (e.g.,
`Chart`), so that `security_manager.can_access("can_read", "Chart")` will
return False
inside `check_tool_permission()` at `superset/mcp_service/auth.py:74-128`.
3. Invoke any async MCP tool that is decorated with `@tool` and
`mcp_auth_hook` (metadata
set in `core_mcp_injection.py` as referenced in the `mcp_auth_hook`
docstring) and has
`CLASS_PERMISSION_ATTR="Chart"` and `METHOD_PERMISSION_ATTR` left at its
default `"read"`.
4. When the call reaches the async wrapper's RBAC block at
`superset/mcp_service/auth.py:355-363`, `check_tool_permission(tool_func)`
returns False,
and `MCPPermissionDeniedError` is raised with `permission_name="read"` even
though the
actual checked permission string was `"can_read"`, leading to a misleading
`Permission
denied: read on Chart` message and incorrect `error.permission_name`
metadata.
```
Prompt for AI Agent 🤖
```mdx
This is a comment left during a code review.
**Path:** superset/mcp_service/auth.py
**Line:** 355:363
**Comment:**
*Logic Error: When RBAC denies access for an async MCP tool, the raised
MCPPermissionDeniedError currently uses the bare action name (e.g. "read" or
"execute_sql_query") instead of the full permission string ("can_read",
"can_execute_sql_query") that was actually checked, which can confuse debugging
and any code/tests that expect the real permission identifier.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
👍
| 👎
##
superset/mcp_service/auth.py:
##
@@ -294,6 +391,17 @@ def sync_wrapper(*args: Any, **kwargs: Any) -> Any:
)
return tool_func(*args, **kwargs)
+# RBAC permission check
+if not check_tool_permission(tool_func):
+raise MCPPermissionDeniedError(
+permission_name=getattr(
+tool_func, METHOD_PERMISSION_ATTR, "read"
+),
+view_name=getattr(tool_func, CLASS_PERMISSION_ATTR,
"unknown"),
+user=user.username,
+tool_name=tool_func.__name__,
+)
Review Comment:
**Suggestion:** For sync MCP tools, MCPPermissionDeniedError is also
constructed with the bare action name instead of the full permission string
("can_*")
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on code in PR #38407: URL: https://github.com/apache/superset/pull/38407#discussion_r2891499552 ## tests/unit_tests/mcp_service/test_auth_rbac.py: ## @@ -0,0 +1,221 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Tests for MCP RBAC permission checking (auth.py).""" + +from unittest.mock import MagicMock, patch + +import pytest +from flask import g + +from superset.mcp_service.auth import ( +check_tool_permission, +CLASS_PERMISSION_ATTR, +MCPPermissionDeniedError, +METHOD_PERMISSION_ATTR, +PERMISSION_PREFIX, +) + + [email protected](autouse=True) +def enable_mcp_rbac(app): +"""Re-enable RBAC for dedicated RBAC tests. + +The shared conftest disables RBAC for integration tests. This fixture +overrides that so we can test the actual permission checking logic. +""" +app.config["MCP_RBAC_ENABLED"] = True +yield +app.config.pop("MCP_RBAC_ENABLED", None) + + +class _ToolFunc: +"""Minimal callable stand-in for a tool function in tests. + +Unlike MagicMock, this does NOT auto-create attributes on access, +so ``getattr(func, ATTR, None)`` properly returns None when the +attribute hasn't been set. +""" + +def __init__(self, name: str = "test_tool"): +self.__name__ = name + +def __call__(self, *args, **kwargs): +pass + + +def _make_tool_func( +class_perm: str | None = None, +method_perm: str | None = None, +) -> _ToolFunc: +"""Create a tool function stub with optional permission attributes.""" +func = _ToolFunc() +if class_perm is not None: +setattr(func, CLASS_PERMISSION_ATTR, class_perm) +if method_perm is not None: +setattr(func, METHOD_PERMISSION_ATTR, method_perm) +return func + + +# -- MCPPermissionDeniedError -- + + +def test_permission_denied_error_message_basic(): +err = MCPPermissionDeniedError( +permission_name="can_read", +view_name="Chart", +) +assert "can_read" in str(err) +assert "Chart" in str(err) +assert err.permission_name == "can_read" +assert err.view_name == "Chart" + + +def test_permission_denied_error_message_with_user_and_tool(): +err = MCPPermissionDeniedError( +permission_name="can_write", +view_name="Dashboard", +user="alice", +tool_name="generate_dashboard", +) +assert "alice" in str(err) +assert "generate_dashboard" in str(err) +assert "Dashboard" in str(err) + + +# -- check_tool_permission -- + + +def test_check_tool_permission_no_class_permission_allows(app_context): +"""Tools without class_permission_name should be allowed by default.""" +g.user = MagicMock(username="admin") +func = _make_tool_func() # no class_permission_name +assert check_tool_permission(func) is True + + +def test_check_tool_permission_no_user_denies(app_context): +"""If no g.user, permission check should deny.""" +g.user = None +func = _make_tool_func(class_perm="Chart") +assert check_tool_permission(func) is False + + +def test_check_tool_permission_granted(app_context): +"""When security_manager.can_access returns True, permission is granted.""" +g.user = MagicMock(username="admin") +func = _make_tool_func(class_perm="Chart", method_perm="read") + +with patch("superset.extensions.appbuilder") as mock_appbuilder: +mock_appbuilder.sm.can_access.return_value = True +result = check_tool_permission(func) + +assert result is True +mock_appbuilder.sm.can_access.assert_called_once_with("can_read", "Chart") + + +def test_check_tool_permission_denied(app_context): +"""When security_manager.can_access returns False, permission is denied.""" +g.user = MagicMock(username="viewer") +func = _make_tool_func(class_perm="Dashboard", method_perm="write") + +with patch("superset.extensions.appbuilder") as mock_appbuilder: +mock_appbuilder.sm.can_access.return_value = False +result = check_tool_permission(func) + +assert result is False +mock_appbuilder.sm.can_access.assert_called_once_with("can_write", "Dashboard") + + +def test_check_tool_permission_default_method_is_read(app_context): +"""When n
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-4006697377 AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). Bito Usage Guide **Commands** Type the following command in the pull request comment and save the comment. - `/review` - Manually triggers a full AI review. - `/pause` - Pauses automatic reviews on this pull request. - `/resume` - Resumes automatic reviews. - `/resolve` - Marks all Bito-posted review comments as resolved. - `/abort` - Cancels all in-progress reviews. Refer to the https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands"; target="_blank" rel="noopener noreferrer">documentation for additional commands. **Configuration** This repository uses `Superset` You can customize the agent settings https://alpha.bito.ai/home/ai-agents/code-review-agent"; target="_blank" rel="noopener noreferrer">here or contact your Bito workspace admin at [email protected]. **Documentation & Help** - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance"; target="_blank" rel="noopener noreferrer">Customize agent settings - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules"; target="_blank" rel="noopener noreferrer">Review rules - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent"; target="_blank" rel="noopener noreferrer">General documentation - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs"; target="_blank" rel="noopener noreferrer">FAQ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on code in PR #38407: URL: https://github.com/apache/superset/pull/38407#discussion_r2889066478 ## superset/mcp_service/chart/tool/update_chart_preview.py: ## @@ -44,7 +44,7 @@ logger = logging.getLogger(__name__) -@tool(tags=["mutate"]) +@tool(tags=["mutate"], class_permission_name="Chart") Review Comment: Missing write permission for mutate tool The tool is tagged "mutate" but lacks method_permission_name, defaulting to "read" permission check. For mutate operations that modify chart data, it should specify method_permission_name="write" to require write access instead. Code suggestion Check the AI-generated fix before applying suggestion @tool(tags=["mutate"], class_permission_name="Chart", method_permission_name="write") Code Review Run #cad346 --- Should Bito avoid suggestions like this for future reviews? (https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules) - [ ] Yes, avoid them -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-4003847056 AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). Bito Usage Guide **Commands** Type the following command in the pull request comment and save the comment. - `/review` - Manually triggers a full AI review. - `/pause` - Pauses automatic reviews on this pull request. - `/resume` - Resumes automatic reviews. - `/resolve` - Marks all Bito-posted review comments as resolved. - `/abort` - Cancels all in-progress reviews. Refer to the https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands"; target="_blank" rel="noopener noreferrer">documentation for additional commands. **Configuration** This repository uses `Superset` You can customize the agent settings https://alpha.bito.ai/home/ai-agents/code-review-agent"; target="_blank" rel="noopener noreferrer">here or contact your Bito workspace admin at [email protected]. **Documentation & Help** - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance"; target="_blank" rel="noopener noreferrer">Customize agent settings - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules"; target="_blank" rel="noopener noreferrer">Review rules - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent"; target="_blank" rel="noopener noreferrer">General documentation - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs"; target="_blank" rel="noopener noreferrer">FAQ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-4001238909 AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). Bito Usage Guide **Commands** Type the following command in the pull request comment and save the comment. - `/review` - Manually triggers a full AI review. - `/pause` - Pauses automatic reviews on this pull request. - `/resume` - Resumes automatic reviews. - `/resolve` - Marks all Bito-posted review comments as resolved. - `/abort` - Cancels all in-progress reviews. Refer to the https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands"; target="_blank" rel="noopener noreferrer">documentation for additional commands. **Configuration** This repository uses `Superset` You can customize the agent settings https://alpha.bito.ai/home/ai-agents/code-review-agent"; target="_blank" rel="noopener noreferrer">here or contact your Bito workspace admin at [email protected]. **Documentation & Help** - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance"; target="_blank" rel="noopener noreferrer">Customize agent settings - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules"; target="_blank" rel="noopener noreferrer">Review rules - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent"; target="_blank" rel="noopener noreferrer">General documentation - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs"; target="_blank" rel="noopener noreferrer">FAQ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
netlify[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-4001003737 ### ✅ Deploy Preview for *superset-docs-preview* ready! | Name | Link | |:-:|| |🔨 Latest commit | 72598ced4f097e3ff95575b0b7ea1e9d267eaab6 | |🔍 Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69a8c2b96f5ce70008361bfe | |😎 Deploy Preview | [https://deploy-preview-38407--superset-docs-preview.netlify.app](https://deploy-preview-38407--superset-docs-preview.netlify.app) | |📱 Preview on mobile | Toggle QR Code... _Use your smartphone camera to open QR code link._ | --- _To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-4000491592 AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). Bito Usage Guide **Commands** Type the following command in the pull request comment and save the comment. - `/review` - Manually triggers a full AI review. - `/pause` - Pauses automatic reviews on this pull request. - `/resume` - Resumes automatic reviews. - `/resolve` - Marks all Bito-posted review comments as resolved. - `/abort` - Cancels all in-progress reviews. Refer to the https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands"; target="_blank" rel="noopener noreferrer">documentation for additional commands. **Configuration** This repository uses `Superset` You can customize the agent settings https://alpha.bito.ai/home/ai-agents/code-review-agent"; target="_blank" rel="noopener noreferrer">here or contact your Bito workspace admin at [email protected]. **Documentation & Help** - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance"; target="_blank" rel="noopener noreferrer">Customize agent settings - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules"; target="_blank" rel="noopener noreferrer">Review rules - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent"; target="_blank" rel="noopener noreferrer">General documentation - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs"; target="_blank" rel="noopener noreferrer">FAQ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-463309 AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). Bito Usage Guide **Commands** Type the following command in the pull request comment and save the comment. - `/review` - Manually triggers a full AI review. - `/pause` - Pauses automatic reviews on this pull request. - `/resume` - Resumes automatic reviews. - `/resolve` - Marks all Bito-posted review comments as resolved. - `/abort` - Cancels all in-progress reviews. Refer to the https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands"; target="_blank" rel="noopener noreferrer">documentation for additional commands. **Configuration** This repository uses `Superset` You can customize the agent settings https://alpha.bito.ai/home/ai-agents/code-review-agent"; target="_blank" rel="noopener noreferrer">here or contact your Bito workspace admin at [email protected]. **Documentation & Help** - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance"; target="_blank" rel="noopener noreferrer">Customize agent settings - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules"; target="_blank" rel="noopener noreferrer">Review rules - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent"; target="_blank" rel="noopener noreferrer">General documentation - https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs"; target="_blank" rel="noopener noreferrer">FAQ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
codecov[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-3999737392 ## [Codecov](https://app.codecov.io/gh/apache/superset/pull/38407?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 64.76%. Comparing base ([`f2f5559`](https://app.codecov.io/gh/apache/superset/commit/f2f55591eccef795f816043b93876e6ca4beacd3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`0b3ebec`](https://app.codecov.io/gh/apache/superset/commit/0b3ebec4291519bed7c91bd0f96cbf3ecb09ddbc?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). :warning: Report is 26 commits behind head on master. :x: Your project check has failed because the head coverage (99.24%) is below the target coverage (100.00%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage. Additional details and impacted files ```diff @@Coverage Diff @@ ## master #38407 +/- ## == + Coverage 64.29% 64.76% +0.46% == Files1811 1819 +8 Lines 7149172335 +844 Branches2277522881 +106 == + Hits4596746849 +882 + Misses 2552425486 -38 ``` | [Flag](https://app.codecov.io/gh/apache/superset/pull/38407/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [superset-extensions-cli](https://app.codecov.io/gh/apache/superset/pull/38407/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `89.94% <ø> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/38407?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. - :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
codeant-ai-for-open-source[bot] commented on code in PR #38407:
URL: https://github.com/apache/superset/pull/38407#discussion_r2885589825
##
tests/unit_tests/mcp_service/test_auth_rbac.py:
##
@@ -0,0 +1,197 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Tests for MCP RBAC permission checking (auth.py)."""
+
+from unittest.mock import MagicMock, patch
+
+import pytest
+from flask import g
+
+from superset.mcp_service.auth import (
+check_tool_permission,
+CLASS_PERMISSION_ATTR,
+MCPPermissionDeniedError,
+METHOD_PERMISSION_ATTR,
+PERMISSION_PREFIX,
+)
+
+
+class _ToolFunc:
+"""Minimal callable stand-in for a tool function in tests.
+
+Unlike MagicMock, this does NOT auto-create attributes on access,
+so ``getattr(func, ATTR, None)`` properly returns None when the
+attribute hasn't been set.
+"""
+
+def __init__(self, name: str = "test_tool"):
+self.__name__ = name
+
+def __call__(self, *args, **kwargs):
+pass
+
+
+def _make_tool_func(
+class_perm: str | None = None,
+method_perm: str | None = None,
+) -> _ToolFunc:
+"""Create a tool function stub with optional permission attributes."""
+func = _ToolFunc()
+if class_perm is not None:
+setattr(func, CLASS_PERMISSION_ATTR, class_perm)
+if method_perm is not None:
+setattr(func, METHOD_PERMISSION_ATTR, method_perm)
+return func
+
+
+# -- MCPPermissionDeniedError --
+
+
+def test_permission_denied_error_message_basic():
+err = MCPPermissionDeniedError(
+permission_name="can_read",
+view_name="Chart",
+)
+assert "can_read" in str(err)
+assert "Chart" in str(err)
+assert err.permission_name == "can_read"
+assert err.view_name == "Chart"
+
+
+def test_permission_denied_error_message_with_user_and_tool():
+err = MCPPermissionDeniedError(
+permission_name="can_write",
+view_name="Dashboard",
+user="alice",
+tool_name="generate_dashboard",
+)
+assert "alice" in str(err)
+assert "generate_dashboard" in str(err)
+assert "Dashboard" in str(err)
+
+
+# -- check_tool_permission --
+
+
+def test_check_tool_permission_no_class_permission_allows(app_context):
+"""Tools without class_permission_name should be allowed by default."""
+g.user = MagicMock(username="admin")
+func = _make_tool_func() # no class_permission_name
+assert check_tool_permission(func) is True
+
+
+def test_check_tool_permission_no_user_denies(app_context):
+"""If no g.user, permission check should deny."""
+g.user = None
+func = _make_tool_func(class_perm="Chart")
+assert check_tool_permission(func) is False
+
+
+def test_check_tool_permission_granted(app_context):
+"""When security_manager.can_access returns True, permission is granted."""
+g.user = MagicMock(username="admin")
+func = _make_tool_func(class_perm="Chart", method_perm="read")
+
+with patch("superset.extensions.appbuilder") as mock_appbuilder:
+mock_appbuilder.sm.can_access.return_value = True
+result = check_tool_permission(func)
+
+assert result is True
+mock_appbuilder.sm.can_access.assert_called_once_with("can_read", "Chart")
+
+
+def test_check_tool_permission_denied(app_context):
+"""When security_manager.can_access returns False, permission is denied."""
+g.user = MagicMock(username="viewer")
+func = _make_tool_func(class_perm="Dashboard", method_perm="write")
+
+with patch("superset.extensions.appbuilder") as mock_appbuilder:
+mock_appbuilder.sm.can_access.return_value = False
+result = check_tool_permission(func)
+
+assert result is False
+mock_appbuilder.sm.can_access.assert_called_once_with("can_write",
"Dashboard")
+
+
+def test_check_tool_permission_default_method_is_read(app_context):
+"""When no method_permission_name is set, defaults to 'read'."""
+g.user = MagicMock(username="admin")
+func = _make_tool_func(class_perm="Dataset")
+# No method_perm set - should default to "read"
+
+with patch("superset.extensions.appbuilder") as mock_appbuilder:
+mock_appbuilder.sm.can_access.return_value = True
+check_tool_permissi
Re: [PR] feat(mcp): implement RBAC permission checking for MCP tools [superset]
bito-code-review[bot] commented on PR #38407: URL: https://github.com/apache/superset/pull/38407#issuecomment-3999601063 AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
