korbit-ai[bot] commented on code in PR #36093:
URL: https://github.com/apache/superset/pull/36093#discussion_r2520547340
##########
superset/mcp_service/auth.py:
##########
@@ -46,39 +46,21 @@ def get_user_from_request() -> User:
"""
Get the current user for the MCP tool request.
- TODO (future PR): Add JWT token extraction and validation.
- TODO (future PR): Add user impersonation support.
- TODO (future PR): Add fallback user configuration.
-
- For now, this returns the admin user for development.
+ The user should already be set by WorkspaceContextMiddleware.
+ This function validates that authentication succeeded.
"""
- from flask import current_app
- from sqlalchemy.orm import joinedload
-
- from superset.extensions import db
-
- # TODO: Extract from JWT token once authentication is implemented
- # For now, use MCP_DEV_USERNAME from configuration
- username = current_app.config.get("MCP_DEV_USERNAME")
-
- if not username:
- raise ValueError("Username not configured")
-
- # Query user directly with eager loading to ensure fresh session-bound
object
- # Do NOT use security_manager.find_user() as it may return cached/detached
user
- user = (
- db.session.query(User)
- .options(joinedload(User.roles), joinedload(User.groups))
- .filter(User.username == username)
- .first()
- )
-
- if not user:
+ # Check if user was set by middleware
+ if not hasattr(g, "user") or g.user is None:
raise ValueError(
- f"User '{username}' not found. "
- f"Please create admin user with: superset fab create-admin"
+ "User not authenticated. This tool requires authentication via JWT
token."
)
+ user = g.user
+
+ # Validate user is properly loaded with relationships
+ if not hasattr(user, "roles"):
+ logger.warning("User object missing 'roles' relationship")
Review Comment:
### Missing roles relationship not handled as error <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code logs a warning but continues execution when the user object is
missing the critical 'roles' relationship, which could lead to authorization
failures downstream.
###### Why this matters
If the user object lacks the 'roles' relationship, any subsequent permission
checks that depend on user roles will fail or behave unpredictably, potentially
causing runtime errors or security issues.
###### Suggested change ∙ *Feature Preview*
Change the warning to raise an exception to prevent execution with an
incomplete user object:
```python
if not hasattr(user, "roles"):
raise ValueError("User object missing required 'roles' relationship")
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4c5c7df6-0c82-47cc-acfc-b7e7d4219798/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4c5c7df6-0c82-47cc-acfc-b7e7d4219798?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4c5c7df6-0c82-47cc-acfc-b7e7d4219798?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4c5c7df6-0c82-47cc-acfc-b7e7d4219798?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/4c5c7df6-0c82-47cc-acfc-b7e7d4219798)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6fe6915d-abf0-4941-9ac3-229953504273 -->
[](6fe6915d-abf0-4941-9ac3-229953504273)
##########
superset/mcp_service/auth.py:
##########
@@ -119,31 +101,28 @@ 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
+ This decorator assumes Flask application context and g.user
+ have already been set by WorkspaceContextMiddleware.
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
"""
import functools
@functools.wraps(tool_func)
def wrapper(*args: Any, **kwargs: Any) -> Any:
from superset.extensions import db
- # Get user and set Flask context OUTSIDE try block
+ # Get user from g (already set by middleware)
user = get_user_from_request()
- # Force load relationships NOW while session is definitely active
+ # Validate user has necessary relationships loaded
+ # (Force access to ensure they're loaded if lazy)
_ = user.roles
if hasattr(user, "groups"):
_ = user.groups
Review Comment:
### Unhandled database errors in relationship loading <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code attempts to force-load user relationships but doesn't handle
potential database errors or lazy loading failures that could occur during
attribute access.
###### Why this matters
If the database session is invalid or the relationships fail to load,
accessing these attributes could raise SQLAlchemy exceptions, causing the
decorated function to fail unexpectedly.
###### Suggested change ∙ *Feature Preview*
Wrap the relationship access in a try-catch block to handle potential
database errors:
```python
try:
_ = user.roles
if hasattr(user, "groups"):
_ = user.groups
except Exception as e:
logger.error("Failed to load user relationships: %s", e)
raise ValueError("User object relationships could not be loaded") from e
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fbaccd1b-df06-4da6-bee7-7b70734ecb8b/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fbaccd1b-df06-4da6-bee7-7b70734ecb8b?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fbaccd1b-df06-4da6-bee7-7b70734ecb8b?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fbaccd1b-df06-4da6-bee7-7b70734ecb8b?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fbaccd1b-df06-4da6-bee7-7b70734ecb8b)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b55d4295-aaa7-4fa5-827f-afeb4a800815 -->
[](b55d4295-aaa7-4fa5-827f-afeb4a800815)
##########
superset/mcp_service/auth.py:
##########
@@ -119,31 +101,28 @@ 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
+ This decorator assumes Flask application context and g.user
+ have already been set by WorkspaceContextMiddleware.
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
"""
import functools
@functools.wraps(tool_func)
def wrapper(*args: Any, **kwargs: Any) -> Any:
from superset.extensions import db
- # Get user and set Flask context OUTSIDE try block
+ # Get user from g (already set by middleware)
user = get_user_from_request()
- # Force load relationships NOW while session is definitely active
+ # Validate user has necessary relationships loaded
+ # (Force access to ensure they're loaded if lazy)
_ = 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?
Unnecessary forced loading of user relationships on every tool call, even
when these relationships may not be used by the tool function.
###### Why this matters
This creates unnecessary database queries and memory overhead for every MCP
tool invocation, potentially causing performance degradation under high load
when many tools don't require role or group information.
###### Suggested change ∙ *Feature Preview*
Move the relationship loading to be lazy or conditional based on actual
usage. Consider adding a parameter to the decorator or checking if the tool
function actually needs these relationships before loading them:
```python
# Only load relationships if needed by the specific tool
if getattr(tool_func, '_requires_roles', False):
_ = user.roles
if getattr(tool_func, '_requires_groups', False) and hasattr(user, "groups"):
_ = user.groups
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a684446c-cf36-41bc-a805-bf75546e1132/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a684446c-cf36-41bc-a805-bf75546e1132?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a684446c-cf36-41bc-a805-bf75546e1132?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a684446c-cf36-41bc-a805-bf75546e1132?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a684446c-cf36-41bc-a805-bf75546e1132)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:49bee786-8109-464c-84be-21604aceb7ac -->
[](49bee786-8109-464c-84be-21604aceb7ac)
--
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]
