korbit-ai[bot] commented on code in PR #36013:
URL: https://github.com/apache/superset/pull/36013#discussion_r2496867344
##########
superset/mcp_service/common/error_schemas.py:
##########
@@ -48,9 +48,11 @@ class ValidationError(BaseModel):
class DatasetContext(BaseModel):
"""Dataset information for error context"""
+ model_config = {"populate_by_name": True}
+
id: int = Field(..., description="Dataset ID")
table_name: str = Field(..., description="Table name")
- schema: str | None = Field(None, description="Schema name")
+ schema_name: str | None = Field(None, alias="schema", description="Schema
name")
Review Comment:
### Incomplete alias configuration for schema field <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The field aliasing configuration may not work as expected without proper
model configuration for backward compatibility.
###### Why this matters
While `populate_by_name` is set to True, this only allows population by
field name. The alias 'schema' might not be properly handled during
serialization, potentially breaking existing code that expects the field to be
serialized as 'schema' rather than 'schema_name'.
###### Suggested change ∙ *Feature Preview*
Add `serialization_alias` to ensure the field is serialized with the
expected name:
```python
schema_name: str | None = Field(
None,
alias="schema",
serialization_alias="schema",
description="Schema name"
)
```
Or use `AliasChoices` for more robust handling:
```python
from pydantic import AliasChoices
schema_name: str | None = Field(
None,
validation_alias=AliasChoices("schema", "schema_name"),
serialization_alias="schema",
description="Schema name"
)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a461dc14-a353-4e8d-8949-d854c1e87fe6/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a461dc14-a353-4e8d-8949-d854c1e87fe6?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a461dc14-a353-4e8d-8949-d854c1e87fe6?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a461dc14-a353-4e8d-8949-d854c1e87fe6?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a461dc14-a353-4e8d-8949-d854c1e87fe6)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0dedeb72-b11c-46ab-a348-40641702bf03 -->
[](0dedeb72-b11c-46ab-a348-40641702bf03)
##########
superset/mcp_service/auth.py:
##########
@@ -119,61 +154,92 @@ def mcp_auth_hook(tool_func: F) -> F:
"""
Authentication and authorization decorator for MCP tools.
- This is a minimal implementation that:
- 1. Gets the current user
- 2. Sets g.user for Flask context
-
- TODO (future PR): Add permission checking
- TODO (future PR): Add JWT scope validation
- TODO (future PR): Add comprehensive audit logging
- TODO (future PR): Add rate limiting integration
+ This hook ensures proper Flask app context and user authentication for all
+ MCP tool calls. Deployers can extend this decorator to add:
+
+ - Permission checks: Validate user has specific roles or tool access rights
+ - JWT scope validation: Check OAuth scopes match required tool permissions
+ - Audit logging: Track tool usage, parameters, and results for compliance
+ - Rate limiting: Enforce per-user or per-tool rate limits
+ - Custom middleware: Inject organization-specific security policies
+
+ Default behavior:
+ 1. Ensures Flask app context is active (critical for stateless_http mode)
+ 2. Authenticates user via get_user_from_request()
+ 3. Sets g.user for Superset's security manager integration
+ 4. Manages database session lifecycle
+
+ Example extensions:
+ # Add permission check before tool execution:
+ if tool_func.__name__ in ['execute_sql'] and not user.is_admin():
+ raise PermissionError("SQL execution requires admin role")
+
+ # Add audit logging after successful execution:
+ audit_log.info({
+ 'user': user.username,
+ 'tool': tool_func.__name__,
+ 'timestamp': datetime.now(),
+ 'result': 'success'
+ })
"""
import functools
@functools.wraps(tool_func)
def wrapper(*args: Any, **kwargs: Any) -> Any:
from superset.extensions import db
+ from superset.mcp_service.flask_singleton import get_flask_app
- # Get user and set Flask context OUTSIDE try block
- user = get_user_from_request()
+ # Get Flask app instance
+ app = get_flask_app()
- # Force load relationships NOW while session is definitely active
- _ = user.roles
- if hasattr(user, "groups"):
- _ = user.groups
+ # Ensure Flask app context is active
+ # This is critical for stateless_http mode where context isn't
preserved
+ with app.app_context():
+ # Get user and set Flask context OUTSIDE try block
+ user = get_user_from_request()
- g.user = user
+ # Force load relationships NOW while session is definitely active
+ _ = user.roles
+ if hasattr(user, "groups"):
+ _ = user.groups
Review Comment:
### Unnecessary eager loading of user relationships <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Eager loading of user relationships on every tool call regardless of whether
they are needed by the specific tool.
###### Why this matters
This forces loading of potentially large relationship collections (roles,
groups) even when tools don't require this data, wasting memory and database
bandwidth.
###### Suggested change ∙ *Feature Preview*
Load relationships lazily or only when needed by specific tools. Consider
adding a parameter to control relationship loading:
```python
def get_user_from_request(load_relationships: bool = False) -> User:
user = db.session.query(User).filter(User.username == username).first()
if load_relationships:
# Only load when explicitly requested
_ = user.roles
if hasattr(user, "groups"):
_ = user.groups
return user
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f55dc8aa-c38a-41ad-9119-7e514359a9a2/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f55dc8aa-c38a-41ad-9119-7e514359a9a2?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f55dc8aa-c38a-41ad-9119-7e514359a9a2?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f55dc8aa-c38a-41ad-9119-7e514359a9a2?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f55dc8aa-c38a-41ad-9119-7e514359a9a2)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:73a2e9d1-60d0-41df-a4b2-bc21c036015b -->
[](73a2e9d1-60d0-41df-a4b2-bc21c036015b)
--
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]
