aminghadersohi commented on code in PR #37973:
URL: https://github.com/apache/superset/pull/37973#discussion_r2819542792
##########
superset/mcp_service/auth.py:
##########
@@ -107,14 +116,32 @@ def get_user_from_request() -> User:
if hasattr(g, "user") and g.user:
return g.user
+ # Try API key authentication via FAB SecurityManager
+ # Only attempt when in a request context (not for MCP internal operations
+ # like tool discovery that run with only an application context)
+ from flask import has_request_context
+
+ api_key_enabled = current_app.config.get("FAB_API_KEY_ENABLED", False)
+ if api_key_enabled and has_request_context():
+ sm = current_app.appbuilder.sm
+ api_key_string = sm._extract_api_key_from_request()
+ if api_key_string is not None:
+ user = sm.validate_api_key(api_key_string)
+ if user:
+ return user
Review Comment:
Good catch — this is a valid concern. The user from `validate_api_key()` may
have lazily loaded relationships, which can cause `DetachedInstanceError` if
the session is rolled back later during MCP tool execution.
Fixed by reloading the user via `load_user_with_relationships()` after
successful API key validation, consistent with how the `MCP_DEV_USERNAME`
fallback path already handles this.
##########
tests/unit_tests/security/api_test.py:
##########
@@ -29,6 +29,7 @@ def test_csrf_not_exempt(app_context: None) -> None:
Test that REST API is not exempt from CSRF.
"""
assert {blueprint.name for blueprint in csrf._exempt_blueprints} == {
+ "ApiKeyApi",
Review Comment:
Thanks for flagging this. The CSRF exemption for `ApiKeyApi` is set by FAB
itself — all FAB security API blueprints (`SecurityApi`, `MenuApi`,
`PermissionApi`, etc.) are registered as CSRF-exempt by FAB's
`register_views()` method. This is consistent behavior across the framework.
The CSRF risk is mitigated because:
1. The API key endpoints require JWT Bearer token auth via `@protect()` — a
cross-site request from a browser wouldn't include a valid JWT
2. Session cookies alone are insufficient to access these endpoints
3. The exemption pattern matches all other FAB security APIs already in
production
If we want to change this behavior, it would need to be addressed in FAB
itself (not Superset), since FAB controls how its blueprints are registered.
For now, the test documents the actual state. Happy to open a separate FAB
issue to evaluate adding CSRF protection to security API blueprints if the
community thinks it's warranted.
--
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]
