aminghadersohi commented on code in PR #39604:
URL: https://github.com/apache/superset/pull/39604#discussion_r3244924722
##########
superset/mcp_service/mcp_config.py:
##########
@@ -335,6 +335,21 @@
auth_provider = JWTVerifier(**common_kwargs)
+ # Wrap with CompositeTokenVerifier when API key auth is enabled
+ # so that API key tokens (e.g. sst_...) pass through the transport
+ # layer instead of being rejected by the JWT verifier.
+ if app.config.get("FAB_API_KEY_ENABLED", False):
+ from superset.mcp_service.composite_token_verifier import (
+ CompositeTokenVerifier,
+ )
+
Review Comment:
Addressed.
##########
tests/unit_tests/mcp_service/test_auth_api_key.py:
##########
@@ -220,3 +223,74 @@ def
test_relationship_reload_failure_returns_original_user(app, mock_user) -> No
result = get_user_from_request()
assert result is mock_user
+
+
+# -- Bearer token present but not matching API key prefix --
+
+
[email protected]("_enable_api_keys")
+def test_non_matching_bearer_token_skips_api_key_auth(app) -> None:
Review Comment:
Addressed.
##########
tests/unit_tests/mcp_service/test_auth_api_key.py:
##########
@@ -220,3 +223,74 @@ def
test_relationship_reload_failure_returns_original_user(app, mock_user) -> No
result = get_user_from_request()
assert result is mock_user
+
+
+# -- Bearer token present but not matching API key prefix --
+
+
[email protected]("_enable_api_keys")
+def test_non_matching_bearer_token_skips_api_key_auth(app) -> None:
+ """When a Bearer token is present but does not match FAB_API_KEY_PREFIXES
+ (e.g., a JWT token), extract_api_key_from_request returns None and API key
+ auth is skipped, falling through to the next auth method."""
+ mock_sm = MagicMock()
+ mock_sm.extract_api_key_from_request.return_value = None
+
+ with app.test_request_context(
+ headers={"Authorization": "Bearer eyJhbGciOiJIUzI1NiJ9.not-an-api-key"}
+ ):
+ g.user = None
+ app.appbuilder = MagicMock()
+ app.appbuilder.sm = mock_sm
+
+ with pytest.raises(ValueError, match="No authenticated user found"):
+ get_user_from_request()
+
+ # extract was called but returned None, so validate should NOT be called
+ mock_sm.extract_api_key_from_request.assert_called_once()
+ mock_sm.validate_api_key.assert_not_called()
+
+
+# -- API key pass-through from CompositeTokenVerifier --
+
+
+def test_jwt_context_with_api_key_passthrough_returns_none(app) -> None:
Review Comment:
Addressed.
##########
tests/unit_tests/mcp_service/test_auth_api_key.py:
##########
@@ -220,3 +223,74 @@ def
test_relationship_reload_failure_returns_original_user(app, mock_user) -> No
result = get_user_from_request()
assert result is mock_user
+
+
+# -- Bearer token present but not matching API key prefix --
+
+
[email protected]("_enable_api_keys")
+def test_non_matching_bearer_token_skips_api_key_auth(app) -> None:
+ """When a Bearer token is present but does not match FAB_API_KEY_PREFIXES
+ (e.g., a JWT token), extract_api_key_from_request returns None and API key
+ auth is skipped, falling through to the next auth method."""
+ mock_sm = MagicMock()
+ mock_sm.extract_api_key_from_request.return_value = None
+
+ with app.test_request_context(
+ headers={"Authorization": "Bearer eyJhbGciOiJIUzI1NiJ9.not-an-api-key"}
+ ):
+ g.user = None
+ app.appbuilder = MagicMock()
+ app.appbuilder.sm = mock_sm
+
+ with pytest.raises(ValueError, match="No authenticated user found"):
+ get_user_from_request()
+
+ # extract was called but returned None, so validate should NOT be called
+ mock_sm.extract_api_key_from_request.assert_called_once()
+ mock_sm.validate_api_key.assert_not_called()
+
+
+# -- API key pass-through from CompositeTokenVerifier --
+
+
+def test_jwt_context_with_api_key_passthrough_returns_none(app) -> None:
+ """When CompositeTokenVerifier passes through an API key token,
+ _resolve_user_from_jwt_context should detect the _api_key_passthrough
+ claim and return None so get_user_from_request falls through to
+ _resolve_user_from_api_key."""
+ mock_access_token = MagicMock()
+ mock_access_token.claims = {"_api_key_passthrough": True}
+
+ with patch(
+ "fastmcp.server.dependencies.get_access_token",
+ return_value=mock_access_token,
+ ):
+ result = _resolve_user_from_jwt_context(app)
+
+ assert result is None
+
+
+# -- SecurityManager method name regression test --
+
+
+def test_security_manager_has_expected_api_key_methods(app) -> None:
Review Comment:
Addressed.
##########
tests/unit_tests/mcp_service/test_composite_token_verifier.py:
##########
@@ -0,0 +1,105 @@
+# 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 CompositeTokenVerifier."""
+
+from unittest.mock import AsyncMock, MagicMock
+
+import pytest
+from fastmcp.server.auth import AccessToken
+
+from superset.mcp_service.composite_token_verifier import
CompositeTokenVerifier
+
+
[email protected]
+def mock_jwt_verifier():
Review Comment:
Addressed.
--
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]
