bito-code-review[bot] commented on code in PR #39604:
URL: https://github.com/apache/superset/pull/39604#discussion_r3357279421
##########
tests/unit_tests/mcp_service/test_mcp_config.py:
##########
@@ -224,3 +224,133 @@ def test_get_mcp_config_respects_app_config_override() ->
None:
custom = {"execute_sql", "health_check"}
config = get_mcp_config({"MCP_DISABLED_TOOLS": custom})
assert config["MCP_DISABLED_TOOLS"] == custom
+
+
+def test_build_composite_verifier_string_prefix():
+ """A plain-string FAB_API_KEY_PREFIXES is wrapped into a single-element
list."""
+ from superset.mcp_service.mcp_config import _build_composite_verifier
+
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: (
+ "sst_" if key == "FAB_API_KEY_PREFIXES" else default
+ )
+
+ result = _build_composite_verifier(mock_app, jwt_verifier=None)
+
+ assert result._api_key_prefixes == ("sst_",)
+
+
+def test_build_composite_verifier_list_prefix():
+ """A list FAB_API_KEY_PREFIXES is passed through as-is."""
+ from superset.mcp_service.mcp_config import _build_composite_verifier
+
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: (
+ ["sst_", "api_"] if key == "FAB_API_KEY_PREFIXES" else default
+ )
+
+ result = _build_composite_verifier(mock_app, jwt_verifier=None)
+
+ assert result._api_key_prefixes == ("sst_", "api_")
+
+
+def test_build_composite_verifier_invalid_prefix_falls_back_to_default():
+ """A non-iterable FAB_API_KEY_PREFIXES (e.g. None) falls back to
['sst_']."""
+ from superset.mcp_service.mcp_config import _build_composite_verifier
+
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: (
+ None if key == "FAB_API_KEY_PREFIXES" else default
+ )
+
+ result = _build_composite_verifier(mock_app, jwt_verifier=None)
+
+ assert result._api_key_prefixes == ("sst_",)
+
+
+# -- get_mcp_api_key_enabled --
+
+
+def test_get_mcp_api_key_enabled_explicit_true():
+ """MCP_API_KEY_ENABLED=True returns True regardless of FAB setting."""
+ from superset.mcp_service.mcp_config import get_mcp_api_key_enabled
+
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: (
+ True if key == "MCP_API_KEY_ENABLED" else default
+ )
+
+ assert get_mcp_api_key_enabled(mock_app) is True
Review Comment:
<!-- Bito Reply -->
The suggestion is valid and appropriate. It addresses the need to verify
that the `startup_warning` parameter correctly triggers the expected logging
behavior when `FAB_API_KEY_ENABLED` is set to `True`.
**tests/unit_tests/mcp_service/test_mcp_config.py**
```
def test_get_mcp_api_key_enabled_fab_fallback_logs_startup_warning():
from superset.mcp_service.mcp_config import get_mcp_api_key_enabled
from unittest.mock import MagicMock, patch
mock_app = MagicMock()
mock_app.config.get.side_effect = lambda key, default=None: (
True if key == "FAB_API_KEY_ENABLED" else default
)
with patch("superset.mcp_service.mcp_config.logger") as mock_logger:
assert get_mcp_api_key_enabled(mock_app, startup_warning=True) is
True
mock_logger.warning.assert_called_once()
```
##########
tests/unit_tests/mcp_service/test_mcp_config.py:
##########
@@ -224,3 +224,133 @@ def test_get_mcp_config_respects_app_config_override() ->
None:
custom = {"execute_sql", "health_check"}
config = get_mcp_config({"MCP_DISABLED_TOOLS": custom})
assert config["MCP_DISABLED_TOOLS"] == custom
+
+
+def test_build_composite_verifier_string_prefix():
+ """A plain-string FAB_API_KEY_PREFIXES is wrapped into a single-element
list."""
+ from superset.mcp_service.mcp_config import _build_composite_verifier
+
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: (
+ "sst_" if key == "FAB_API_KEY_PREFIXES" else default
+ )
+
+ result = _build_composite_verifier(mock_app, jwt_verifier=None)
+
+ assert result._api_key_prefixes == ("sst_",)
+
+
+def test_build_composite_verifier_list_prefix():
+ """A list FAB_API_KEY_PREFIXES is passed through as-is."""
+ from superset.mcp_service.mcp_config import _build_composite_verifier
+
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: (
+ ["sst_", "api_"] if key == "FAB_API_KEY_PREFIXES" else default
+ )
+
+ result = _build_composite_verifier(mock_app, jwt_verifier=None)
+
+ assert result._api_key_prefixes == ("sst_", "api_")
+
+
+def test_build_composite_verifier_invalid_prefix_falls_back_to_default():
+ """A non-iterable FAB_API_KEY_PREFIXES (e.g. None) falls back to
['sst_']."""
+ from superset.mcp_service.mcp_config import _build_composite_verifier
+
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: (
+ None if key == "FAB_API_KEY_PREFIXES" else default
+ )
+
+ result = _build_composite_verifier(mock_app, jwt_verifier=None)
+
+ assert result._api_key_prefixes == ("sst_",)
+
+
+# -- get_mcp_api_key_enabled --
+
+
+def test_get_mcp_api_key_enabled_explicit_true():
+ """MCP_API_KEY_ENABLED=True returns True regardless of FAB setting."""
+ from superset.mcp_service.mcp_config import get_mcp_api_key_enabled
+
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: (
+ True if key == "MCP_API_KEY_ENABLED" else default
+ )
+
+ assert get_mcp_api_key_enabled(mock_app) is True
+
+
+def test_get_mcp_api_key_enabled_explicit_false():
+ """MCP_API_KEY_ENABLED=False returns False even when FAB setting is
True."""
+ from superset.mcp_service.mcp_config import get_mcp_api_key_enabled
+
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: (
+ False if key == "MCP_API_KEY_ENABLED" else True
+ )
+
+ assert get_mcp_api_key_enabled(mock_app) is False
+
+
+def test_get_mcp_api_key_enabled_falls_back_to_fab():
+ """When MCP_API_KEY_ENABLED is not set, falls back to
FAB_API_KEY_ENABLED."""
+ from superset.mcp_service.mcp_config import get_mcp_api_key_enabled
+
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: (
+ None
+ if key == "MCP_API_KEY_ENABLED"
+ else (True if key == "FAB_API_KEY_ENABLED" else default)
+ )
+
+ assert get_mcp_api_key_enabled(mock_app) is True
+
+
+def test_get_mcp_api_key_enabled_both_absent_returns_false():
+ """When neither setting is configured, returns False."""
+ from superset.mcp_service.mcp_config import get_mcp_api_key_enabled
+
+ mock_app = MagicMock()
+ mock_app.config.get.return_value = None
+
+ assert get_mcp_api_key_enabled(mock_app) is False
+
+
+# -- create_default_mcp_auth_factory --
+
+
+def test_create_default_mcp_auth_factory_returns_none_when_disabled():
+ """Returns None when neither MCP_AUTH_ENABLED nor API key auth is on."""
+ from superset.mcp_service.mcp_config import create_default_mcp_auth_factory
+
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: (
+ False
+ if key in ("MCP_AUTH_ENABLED", "MCP_API_KEY_ENABLED",
"FAB_API_KEY_ENABLED")
+ else default
+ )
+
+ result = create_default_mcp_auth_factory(mock_app)
+
+ assert result is None
+
+
+def test_create_default_mcp_auth_factory_api_key_only():
+ """Returns a CompositeTokenVerifier when only API key auth is enabled."""
+ from superset.mcp_service.composite_token_verifier import
CompositeTokenVerifier
+ from superset.mcp_service.mcp_config import create_default_mcp_auth_factory
+
+ mock_app = MagicMock()
+ mock_app.config.get.side_effect = lambda key, default=None: {
+ "MCP_AUTH_ENABLED": False,
+ "MCP_API_KEY_ENABLED": True,
+ "FAB_API_KEY_PREFIXES": ["sst_"],
+ "MCP_REQUIRED_SCOPES": [],
+ }.get(key, default)
+
+ result = create_default_mcp_auth_factory(mock_app)
+
+ assert isinstance(result, CompositeTokenVerifier)
Review Comment:
<!-- Bito Reply -->
The pull request includes three new tests in
`tests/unit_tests/mcp_service/test_mcp_config.py` that cover JWT-related
branches of `create_default_mcp_auth_factory`. These tests address the
following scenarios:
* **JWT enabled with keys:** Verifies that the factory returns the built
verifier when JWT is enabled with a secret and API key authentication is
disabled.
* **JWT enabled without keys:** Verifies that the factory returns `None`
and logs a warning when JWT is enabled but no keys/secret are provided and
there is no API-key fallback.
* **JWT build failure:** Verifies that the factory returns `None` and logs
an error when `_build_jwt_verifier` raises a `ValueError` and there is no
API-key fallback.
These additions improve the test coverage for the authentication factory's
configuration logic.
--
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]
