codeant-ai-for-open-source[bot] commented on code in PR #38747: URL: https://github.com/apache/superset/pull/38747#discussion_r2980369571
########## tests/unit_tests/mcp_service/test_auth_user_resolution.py: ########## @@ -0,0 +1,380 @@ +# 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 MCP user resolution priority and stale g.user prevention.""" + +from unittest.mock import MagicMock, patch + +import pytest +from flask import g + +from superset.mcp_service.auth import ( + _resolve_user_from_jwt_context, + get_user_from_request, + mcp_auth_hook, +) +from superset.mcp_service.mcp_config import default_user_resolver + + +def _make_mock_user(username: str = "testuser") -> MagicMock: + """Create a mock User with required attributes.""" + user = MagicMock() + user.username = username + user.roles = [] + user.groups = [] + return user + + +def _make_access_token( + claims: dict[str, str] | None = None, **kwargs: str +) -> MagicMock: + """Create a mock AccessToken matching FastMCP's format.""" + token = MagicMock() + token.claims = claims or {} + token.client_id = kwargs.get("client_id", "") + token.scopes = kwargs.get("scopes", []) + # Remove auto-created attributes so getattr fallbacks work correctly + for attr in ("subject", "payload"): + if attr not in kwargs: + delattr(token, attr) + for attr in kwargs: + setattr(token, attr, kwargs[attr]) + return token + + +# -- _resolve_user_from_jwt_context -- + + +def test_jwt_context_resolves_correct_user(app) -> None: + """JWT context with valid claims resolves the correct DB user.""" + mock_user = _make_mock_user("alice") + token = _make_access_token(claims={"sub": "alice"}) + + with app.app_context(): + with ( + patch("fastmcp.server.dependencies.get_access_token", return_value=token), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=mock_user, + ), + ): + result = _resolve_user_from_jwt_context(app) + + assert result is not None + assert result.username == "alice" + + +def test_jwt_context_returns_none_when_no_token(app) -> None: + """No JWT token present returns None (fall through to next source).""" + with app.app_context(): + with patch("fastmcp.server.dependencies.get_access_token", return_value=None): + result = _resolve_user_from_jwt_context(app) + + assert result is None + + +def test_jwt_context_raises_for_unknown_user(app) -> None: + """JWT resolves a username not in DB — raises ValueError (fail closed).""" + token = _make_access_token(claims={"sub": "nonexistent"}) + + with app.app_context(): + with ( + patch("fastmcp.server.dependencies.get_access_token", return_value=token), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=None, + ), + ): + with pytest.raises(ValueError, match="not found in Superset database"): + _resolve_user_from_jwt_context(app) + + +def test_jwt_context_raises_when_no_username_in_claims(app) -> None: + """JWT present but claims have no extractable username — fails closed.""" + token = _make_access_token(claims={"iss": "some-issuer"}) + + with app.app_context(): + with patch("fastmcp.server.dependencies.get_access_token", return_value=token): + with pytest.raises(ValueError, match="no username could be extracted"): + _resolve_user_from_jwt_context(app) + + +def test_jwt_context_uses_custom_resolver(app) -> None: + """Custom MCP_USER_RESOLVER config is used when set.""" + mock_user = _make_mock_user("custom_user") + token = _make_access_token(claims={"custom_field": "custom_user"}) + custom_resolver = MagicMock(return_value="custom_user") + + with app.app_context(): + app.config["MCP_USER_RESOLVER"] = custom_resolver + try: + with ( + patch( + "fastmcp.server.dependencies.get_access_token", return_value=token + ), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=mock_user, + ), + ): + result = _resolve_user_from_jwt_context(app) + finally: + app.config.pop("MCP_USER_RESOLVER", None) + + assert result is not None + assert result.username == "custom_user" + custom_resolver.assert_called_once_with(app, token) + + +def test_jwt_context_email_fallback_lookup(app) -> None: + """When resolver returns an email, tries email lookup after username miss.""" + mock_user = _make_mock_user("alice") + token = _make_access_token(claims={"email": "[email protected]"}) + + def _load_side_effect(username=None, email=None): + if email == "[email protected]": + return mock_user + return None + + with app.app_context(): + with ( + patch("fastmcp.server.dependencies.get_access_token", return_value=token), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + side_effect=_load_side_effect, + ), + ): + result = _resolve_user_from_jwt_context(app) + + assert result is not None + assert result.username == "alice" + + +# -- get_user_from_request priority order -- + + +def test_jwt_takes_priority_over_stale_g_user(app) -> None: + """Core regression test: JWT user wins over stale g.user.""" + stale_user = _make_mock_user("stale_bob") + jwt_user = _make_mock_user("jwt_alice") + token = _make_access_token(claims={"sub": "jwt_alice"}) + + with app.app_context(): + g.user = stale_user + with ( + patch("fastmcp.server.dependencies.get_access_token", return_value=token), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=jwt_user, + ), + ): + result = get_user_from_request() + + assert result.username == "jwt_alice" + + +def test_dev_username_fallback_when_no_jwt(app) -> None: + """MCP_DEV_USERNAME used when no JWT context available.""" + mock_user = _make_mock_user("dev_admin") + + with app.app_context(): + app.config["MCP_DEV_USERNAME"] = "dev_admin" + try: + with ( + patch( + "fastmcp.server.dependencies.get_access_token", return_value=None + ), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=mock_user, + ), + ): + result = get_user_from_request() + finally: + app.config.pop("MCP_DEV_USERNAME", None) + + assert result.username == "dev_admin" + + +def test_g_user_fallback_when_no_jwt_and_no_dev_username(app) -> None: + """g.user used as last-resort fallback (Preset middleware compatibility).""" + preset_user = _make_mock_user("preset_user") + + with app.app_context(): + app.config.pop("MCP_DEV_USERNAME", None) + g.user = preset_user + with patch("fastmcp.server.dependencies.get_access_token", return_value=None): + result = get_user_from_request() + + assert result.username == "preset_user" + + +def test_raises_when_no_auth_source(app) -> None: + """ValueError raised when no auth source is available.""" + with app.app_context(): + app.config.pop("MCP_DEV_USERNAME", None) + g.pop("user", None) + with patch("fastmcp.server.dependencies.get_access_token", return_value=None): + with pytest.raises(ValueError, match="No authenticated user found"): + get_user_from_request() + + +def test_dev_username_not_found_raises(app) -> None: + """MCP_DEV_USERNAME configured but user not in DB raises ValueError.""" + with app.app_context(): + app.config["MCP_DEV_USERNAME"] = "ghost" + try: + with ( + patch( + "fastmcp.server.dependencies.get_access_token", return_value=None + ), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=None, + ), + ): + with pytest.raises(ValueError, match="not found"): + get_user_from_request() + finally: + app.config.pop("MCP_DEV_USERNAME", None) + + +# -- g.user clearing in mcp_auth_hook -- + + +def test_mcp_auth_hook_clears_stale_g_user(app) -> None: + """mcp_auth_hook clears g.user before setting up user context.""" + stale_user = _make_mock_user("stale") + fresh_user = _make_mock_user("fresh") + + def dummy_tool(): + """Dummy tool.""" + return g.user.username + + wrapped = mcp_auth_hook(dummy_tool) + + with app.app_context(): + g.user = stale_user + with patch( + "superset.mcp_service.auth.get_user_from_request", + return_value=fresh_user, Review Comment: **Suggestion:** This test does not actually verify stale `g.user` is cleared before user resolution. Because `get_user_from_request` is hard-mocked to return `fresh_user`, the assertion still passes even if stale `g.user` is never removed, so a real impersonation regression can slip through. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Security regression can bypass this unit test. - ⚠️ MCP auth wrapper changes may ship unguarded. - ⚠️ False confidence in stale g.user mitigation. ``` </details> ```suggestion def _get_user_from_request_side_effect(): assert not hasattr(g, "user") return fresh_user with patch( "superset.mcp_service.auth.get_user_from_request", side_effect=_get_user_from_request_side_effect, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `superset/mcp_service/auth.py` inside `mcp_auth_hook()` sync wrapper (`sync_wrapper`, lines 149-159 in current file view), remove or break stale cleanup (`g.pop("user", None)` at lines 157-158), leaving stale `g.user` behavior regressed. 2. Run `test_mcp_auth_hook_clears_stale_g_user` in `tests/unit_tests/mcp_service/test_auth_user_resolution.py` (`test_mcp_auth_hook_clears_stale_g_user`, lines 259-278). 3. The test path patches `get_user_from_request` at lines 272-275 to always return `fresh_user`, so `_setup_user_context()` (`superset/mcp_service/auth.py:8-49` in second chunk) sets `g.user = fresh_user` regardless of whether stale `g.user` was cleared first. 4. `dummy_tool()` returns `g.user.username` (line 266), assertion still passes (`"fresh"`), so regression in stale-user clearing is not detected by this test. 5. This is a real blind spot because protected MCP tools/prompts are wrapped by `mcp_auth_hook` in `superset/core/mcp/core_mcp_injection.py` (`wrapped_func = mcp_auth_hook(func)` at lines 30 and 129), so this test is intended to guard security-critical wrapper behavior. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/test_auth_user_resolution.py **Line:** 272:274 **Comment:** *Logic Error: This test does not actually verify stale `g.user` is cleared before user resolution. Because `get_user_from_request` is hard-mocked to return `fresh_user`, the assertion still passes even if stale `g.user` is never removed, so a real impersonation regression can slip through. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=7b0b3709c515b904275f4961bb94e0139370d0feaedb6b77c9a083ba4a2b451f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=7b0b3709c515b904275f4961bb94e0139370d0feaedb6b77c9a083ba4a2b451f&reaction=dislike'>👎</a> ########## tests/unit_tests/mcp_service/test_auth_user_resolution.py: ########## @@ -0,0 +1,380 @@ +# 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 MCP user resolution priority and stale g.user prevention.""" + +from unittest.mock import MagicMock, patch + +import pytest +from flask import g + +from superset.mcp_service.auth import ( + _resolve_user_from_jwt_context, + get_user_from_request, + mcp_auth_hook, +) +from superset.mcp_service.mcp_config import default_user_resolver + + +def _make_mock_user(username: str = "testuser") -> MagicMock: + """Create a mock User with required attributes.""" + user = MagicMock() + user.username = username + user.roles = [] + user.groups = [] + return user + + +def _make_access_token( + claims: dict[str, str] | None = None, **kwargs: str +) -> MagicMock: + """Create a mock AccessToken matching FastMCP's format.""" + token = MagicMock() + token.claims = claims or {} + token.client_id = kwargs.get("client_id", "") + token.scopes = kwargs.get("scopes", []) + # Remove auto-created attributes so getattr fallbacks work correctly + for attr in ("subject", "payload"): + if attr not in kwargs: + delattr(token, attr) + for attr in kwargs: + setattr(token, attr, kwargs[attr]) + return token + + +# -- _resolve_user_from_jwt_context -- + + +def test_jwt_context_resolves_correct_user(app) -> None: + """JWT context with valid claims resolves the correct DB user.""" + mock_user = _make_mock_user("alice") + token = _make_access_token(claims={"sub": "alice"}) + + with app.app_context(): + with ( + patch("fastmcp.server.dependencies.get_access_token", return_value=token), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=mock_user, + ), + ): + result = _resolve_user_from_jwt_context(app) + + assert result is not None + assert result.username == "alice" + + +def test_jwt_context_returns_none_when_no_token(app) -> None: + """No JWT token present returns None (fall through to next source).""" + with app.app_context(): + with patch("fastmcp.server.dependencies.get_access_token", return_value=None): + result = _resolve_user_from_jwt_context(app) + + assert result is None + + +def test_jwt_context_raises_for_unknown_user(app) -> None: + """JWT resolves a username not in DB — raises ValueError (fail closed).""" + token = _make_access_token(claims={"sub": "nonexistent"}) + + with app.app_context(): + with ( + patch("fastmcp.server.dependencies.get_access_token", return_value=token), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=None, + ), + ): + with pytest.raises(ValueError, match="not found in Superset database"): + _resolve_user_from_jwt_context(app) + + +def test_jwt_context_raises_when_no_username_in_claims(app) -> None: + """JWT present but claims have no extractable username — fails closed.""" + token = _make_access_token(claims={"iss": "some-issuer"}) + + with app.app_context(): + with patch("fastmcp.server.dependencies.get_access_token", return_value=token): + with pytest.raises(ValueError, match="no username could be extracted"): + _resolve_user_from_jwt_context(app) + + +def test_jwt_context_uses_custom_resolver(app) -> None: + """Custom MCP_USER_RESOLVER config is used when set.""" + mock_user = _make_mock_user("custom_user") + token = _make_access_token(claims={"custom_field": "custom_user"}) + custom_resolver = MagicMock(return_value="custom_user") + + with app.app_context(): + app.config["MCP_USER_RESOLVER"] = custom_resolver + try: + with ( + patch( + "fastmcp.server.dependencies.get_access_token", return_value=token + ), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=mock_user, + ), + ): + result = _resolve_user_from_jwt_context(app) + finally: + app.config.pop("MCP_USER_RESOLVER", None) + + assert result is not None + assert result.username == "custom_user" + custom_resolver.assert_called_once_with(app, token) + + +def test_jwt_context_email_fallback_lookup(app) -> None: + """When resolver returns an email, tries email lookup after username miss.""" + mock_user = _make_mock_user("alice") + token = _make_access_token(claims={"email": "[email protected]"}) + + def _load_side_effect(username=None, email=None): + if email == "[email protected]": + return mock_user + return None + + with app.app_context(): + with ( + patch("fastmcp.server.dependencies.get_access_token", return_value=token), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + side_effect=_load_side_effect, + ), + ): + result = _resolve_user_from_jwt_context(app) + + assert result is not None + assert result.username == "alice" + + +# -- get_user_from_request priority order -- + + +def test_jwt_takes_priority_over_stale_g_user(app) -> None: + """Core regression test: JWT user wins over stale g.user.""" + stale_user = _make_mock_user("stale_bob") + jwt_user = _make_mock_user("jwt_alice") + token = _make_access_token(claims={"sub": "jwt_alice"}) + + with app.app_context(): + g.user = stale_user + with ( + patch("fastmcp.server.dependencies.get_access_token", return_value=token), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=jwt_user, + ), + ): + result = get_user_from_request() + + assert result.username == "jwt_alice" + + +def test_dev_username_fallback_when_no_jwt(app) -> None: + """MCP_DEV_USERNAME used when no JWT context available.""" + mock_user = _make_mock_user("dev_admin") + + with app.app_context(): + app.config["MCP_DEV_USERNAME"] = "dev_admin" + try: + with ( + patch( + "fastmcp.server.dependencies.get_access_token", return_value=None + ), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=mock_user, + ), + ): + result = get_user_from_request() + finally: + app.config.pop("MCP_DEV_USERNAME", None) + + assert result.username == "dev_admin" + + +def test_g_user_fallback_when_no_jwt_and_no_dev_username(app) -> None: + """g.user used as last-resort fallback (Preset middleware compatibility).""" + preset_user = _make_mock_user("preset_user") + + with app.app_context(): + app.config.pop("MCP_DEV_USERNAME", None) + g.user = preset_user + with patch("fastmcp.server.dependencies.get_access_token", return_value=None): + result = get_user_from_request() + + assert result.username == "preset_user" + + +def test_raises_when_no_auth_source(app) -> None: + """ValueError raised when no auth source is available.""" + with app.app_context(): + app.config.pop("MCP_DEV_USERNAME", None) + g.pop("user", None) + with patch("fastmcp.server.dependencies.get_access_token", return_value=None): + with pytest.raises(ValueError, match="No authenticated user found"): + get_user_from_request() + + +def test_dev_username_not_found_raises(app) -> None: + """MCP_DEV_USERNAME configured but user not in DB raises ValueError.""" + with app.app_context(): + app.config["MCP_DEV_USERNAME"] = "ghost" + try: + with ( + patch( + "fastmcp.server.dependencies.get_access_token", return_value=None + ), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=None, + ), + ): + with pytest.raises(ValueError, match="not found"): + get_user_from_request() + finally: + app.config.pop("MCP_DEV_USERNAME", None) + + +# -- g.user clearing in mcp_auth_hook -- + + +def test_mcp_auth_hook_clears_stale_g_user(app) -> None: + """mcp_auth_hook clears g.user before setting up user context.""" + stale_user = _make_mock_user("stale") + fresh_user = _make_mock_user("fresh") + + def dummy_tool(): + """Dummy tool.""" + return g.user.username + + wrapped = mcp_auth_hook(dummy_tool) + + with app.app_context(): + g.user = stale_user + with patch( + "superset.mcp_service.auth.get_user_from_request", + return_value=fresh_user, + ): + result = wrapped() + + assert result == "fresh" + + +def test_mcp_auth_hook_clears_stale_g_user_async(app) -> None: + """mcp_auth_hook clears g.user before setting up user context (async).""" + import asyncio + + stale_user = _make_mock_user("stale") + fresh_user = _make_mock_user("fresh") + + async def dummy_tool(): + """Dummy tool.""" + return g.user.username + + wrapped = mcp_auth_hook(dummy_tool) + + with app.app_context(): + g.user = stale_user + with patch( + "superset.mcp_service.auth.get_user_from_request", + return_value=fresh_user, Review Comment: **Suggestion:** The async stale-user test has the same masking issue as the sync version: mocking `get_user_from_request` with a fixed return value allows the test to pass even when stale `g.user` is not cleared, so it can miss the exact bug this PR is fixing. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Async auth regressions may evade test coverage. - ⚠️ Stale identity bug could reappear unnoticed. - ⚠️ Protected async MCP tools lose safety checks. ``` </details> ```suggestion def _get_user_from_request_side_effect(): assert not hasattr(g, "user") return fresh_user with patch( "superset.mcp_service.auth.get_user_from_request", side_effect=_get_user_from_request_side_effect, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Regress async stale cleanup in `superset/mcp_service/auth.py` `async_wrapper` by removing/breaking `g.pop("user", None)` (lines 109-110 in current file view). 2. Execute `test_mcp_auth_hook_clears_stale_g_user_async` in `tests/unit_tests/mcp_service/test_auth_user_resolution.py` (lines 281-302). 3. Test patches `get_user_from_request` (lines 296-299) to fixed `fresh_user`; `_setup_user_context()` then sets `g.user = fresh_user` (`auth.py`, second chunk line 48), independent of pre-clear behavior. 4. Wrapped async tool returns `g.user.username` (line 290) and assertion expects `"fresh"` (line 302), so test passes even if stale cleanup never happened. 5. Because MCP wrapping is broadly applied via `core_mcp_injection.py` (`mcp_auth_hook` at lines 30/129), this async test should directly detect precondition, but currently does not. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/test_auth_user_resolution.py **Line:** 296:298 **Comment:** *Logic Error: The async stale-user test has the same masking issue as the sync version: mocking `get_user_from_request` with a fixed return value allows the test to pass even when stale `g.user` is not cleared, so it can miss the exact bug this PR is fixing. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=8171da1c57349132edb2ed3fbc663db4353943ce1cc58696890de30941a797e2&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=8171da1c57349132edb2ed3fbc663db4353943ce1cc58696890de30941a797e2&reaction=dislike'>👎</a> ########## tests/unit_tests/mcp_service/test_auth_user_resolution.py: ########## @@ -0,0 +1,380 @@ +# 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 MCP user resolution priority and stale g.user prevention.""" + +from unittest.mock import MagicMock, patch + +import pytest +from flask import g + +from superset.mcp_service.auth import ( + _resolve_user_from_jwt_context, + get_user_from_request, + mcp_auth_hook, +) +from superset.mcp_service.mcp_config import default_user_resolver + + +def _make_mock_user(username: str = "testuser") -> MagicMock: + """Create a mock User with required attributes.""" + user = MagicMock() + user.username = username + user.roles = [] + user.groups = [] + return user + + +def _make_access_token( + claims: dict[str, str] | None = None, **kwargs: str +) -> MagicMock: + """Create a mock AccessToken matching FastMCP's format.""" + token = MagicMock() + token.claims = claims or {} + token.client_id = kwargs.get("client_id", "") + token.scopes = kwargs.get("scopes", []) + # Remove auto-created attributes so getattr fallbacks work correctly + for attr in ("subject", "payload"): + if attr not in kwargs: + delattr(token, attr) + for attr in kwargs: + setattr(token, attr, kwargs[attr]) + return token + + +# -- _resolve_user_from_jwt_context -- + + +def test_jwt_context_resolves_correct_user(app) -> None: + """JWT context with valid claims resolves the correct DB user.""" + mock_user = _make_mock_user("alice") + token = _make_access_token(claims={"sub": "alice"}) + + with app.app_context(): + with ( + patch("fastmcp.server.dependencies.get_access_token", return_value=token), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=mock_user, + ), + ): + result = _resolve_user_from_jwt_context(app) + + assert result is not None + assert result.username == "alice" + + +def test_jwt_context_returns_none_when_no_token(app) -> None: + """No JWT token present returns None (fall through to next source).""" + with app.app_context(): + with patch("fastmcp.server.dependencies.get_access_token", return_value=None): + result = _resolve_user_from_jwt_context(app) + + assert result is None + + +def test_jwt_context_raises_for_unknown_user(app) -> None: + """JWT resolves a username not in DB — raises ValueError (fail closed).""" + token = _make_access_token(claims={"sub": "nonexistent"}) + + with app.app_context(): + with ( + patch("fastmcp.server.dependencies.get_access_token", return_value=token), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=None, + ), + ): + with pytest.raises(ValueError, match="not found in Superset database"): + _resolve_user_from_jwt_context(app) + + +def test_jwt_context_raises_when_no_username_in_claims(app) -> None: + """JWT present but claims have no extractable username — fails closed.""" + token = _make_access_token(claims={"iss": "some-issuer"}) + + with app.app_context(): + with patch("fastmcp.server.dependencies.get_access_token", return_value=token): + with pytest.raises(ValueError, match="no username could be extracted"): + _resolve_user_from_jwt_context(app) + + +def test_jwt_context_uses_custom_resolver(app) -> None: + """Custom MCP_USER_RESOLVER config is used when set.""" + mock_user = _make_mock_user("custom_user") + token = _make_access_token(claims={"custom_field": "custom_user"}) + custom_resolver = MagicMock(return_value="custom_user") + + with app.app_context(): + app.config["MCP_USER_RESOLVER"] = custom_resolver + try: + with ( + patch( + "fastmcp.server.dependencies.get_access_token", return_value=token + ), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=mock_user, + ), + ): + result = _resolve_user_from_jwt_context(app) + finally: + app.config.pop("MCP_USER_RESOLVER", None) + + assert result is not None + assert result.username == "custom_user" + custom_resolver.assert_called_once_with(app, token) + + +def test_jwt_context_email_fallback_lookup(app) -> None: + """When resolver returns an email, tries email lookup after username miss.""" + mock_user = _make_mock_user("alice") + token = _make_access_token(claims={"email": "[email protected]"}) + + def _load_side_effect(username=None, email=None): + if email == "[email protected]": + return mock_user + return None + + with app.app_context(): + with ( + patch("fastmcp.server.dependencies.get_access_token", return_value=token), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + side_effect=_load_side_effect, + ), + ): + result = _resolve_user_from_jwt_context(app) + + assert result is not None + assert result.username == "alice" + + +# -- get_user_from_request priority order -- + + +def test_jwt_takes_priority_over_stale_g_user(app) -> None: + """Core regression test: JWT user wins over stale g.user.""" + stale_user = _make_mock_user("stale_bob") + jwt_user = _make_mock_user("jwt_alice") + token = _make_access_token(claims={"sub": "jwt_alice"}) + + with app.app_context(): + g.user = stale_user + with ( + patch("fastmcp.server.dependencies.get_access_token", return_value=token), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=jwt_user, + ), + ): + result = get_user_from_request() + + assert result.username == "jwt_alice" + + +def test_dev_username_fallback_when_no_jwt(app) -> None: + """MCP_DEV_USERNAME used when no JWT context available.""" + mock_user = _make_mock_user("dev_admin") + + with app.app_context(): + app.config["MCP_DEV_USERNAME"] = "dev_admin" + try: + with ( + patch( + "fastmcp.server.dependencies.get_access_token", return_value=None + ), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=mock_user, + ), + ): + result = get_user_from_request() + finally: + app.config.pop("MCP_DEV_USERNAME", None) + + assert result.username == "dev_admin" + + +def test_g_user_fallback_when_no_jwt_and_no_dev_username(app) -> None: + """g.user used as last-resort fallback (Preset middleware compatibility).""" + preset_user = _make_mock_user("preset_user") + + with app.app_context(): + app.config.pop("MCP_DEV_USERNAME", None) + g.user = preset_user + with patch("fastmcp.server.dependencies.get_access_token", return_value=None): + result = get_user_from_request() + + assert result.username == "preset_user" + + +def test_raises_when_no_auth_source(app) -> None: + """ValueError raised when no auth source is available.""" + with app.app_context(): + app.config.pop("MCP_DEV_USERNAME", None) + g.pop("user", None) + with patch("fastmcp.server.dependencies.get_access_token", return_value=None): + with pytest.raises(ValueError, match="No authenticated user found"): + get_user_from_request() + + +def test_dev_username_not_found_raises(app) -> None: + """MCP_DEV_USERNAME configured but user not in DB raises ValueError.""" + with app.app_context(): + app.config["MCP_DEV_USERNAME"] = "ghost" + try: + with ( + patch( + "fastmcp.server.dependencies.get_access_token", return_value=None + ), + patch( + "superset.mcp_service.auth.load_user_with_relationships", + return_value=None, + ), + ): + with pytest.raises(ValueError, match="not found"): + get_user_from_request() + finally: + app.config.pop("MCP_DEV_USERNAME", None) + + +# -- g.user clearing in mcp_auth_hook -- + + +def test_mcp_auth_hook_clears_stale_g_user(app) -> None: + """mcp_auth_hook clears g.user before setting up user context.""" + stale_user = _make_mock_user("stale") + fresh_user = _make_mock_user("fresh") + + def dummy_tool(): + """Dummy tool.""" + return g.user.username + + wrapped = mcp_auth_hook(dummy_tool) + + with app.app_context(): + g.user = stale_user + with patch( + "superset.mcp_service.auth.get_user_from_request", + return_value=fresh_user, + ): + result = wrapped() + + assert result == "fresh" + + +def test_mcp_auth_hook_clears_stale_g_user_async(app) -> None: + """mcp_auth_hook clears g.user before setting up user context (async).""" + import asyncio + + stale_user = _make_mock_user("stale") + fresh_user = _make_mock_user("fresh") + + async def dummy_tool(): + """Dummy tool.""" + return g.user.username + + wrapped = mcp_auth_hook(dummy_tool) + + with app.app_context(): + g.user = stale_user + with patch( + "superset.mcp_service.auth.get_user_from_request", + return_value=fresh_user, + ): + result = asyncio.run(wrapped()) + + assert result == "fresh" + + +def test_mcp_auth_hook_preserves_g_user_in_request_context(app) -> None: + """g.user is NOT cleared when a request context is active (middleware compat).""" + middleware_user = _make_mock_user("middleware_user") + + def dummy_tool(): + """Dummy tool.""" + return g.user.username + + wrapped = mcp_auth_hook(dummy_tool) + + with app.test_request_context(): + g.user = middleware_user + with patch( + "superset.mcp_service.auth.get_user_from_request", + return_value=middleware_user, Review Comment: **Suggestion:** This test claims to verify `g.user` is preserved during request context, but the mock always returns `middleware_user` and does not check whether `g.user` was cleared beforehand, so it cannot detect accidental clearing in request-context mode. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Request-context compatibility regressions may go undetected. - ⚠️ Middleware-provided identity handling lacks strict test assertion. - ⚠️ MCP auth behavior changes can pass falsely. ``` </details> ```suggestion def _get_user_from_request_side_effect(): assert hasattr(g, "user") assert g.user is middleware_user return middleware_user with patch( "superset.mcp_service.auth.get_user_from_request", side_effect=_get_user_from_request_side_effect, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Introduce a regression in `superset/mcp_service/auth.py` `sync_wrapper` so it clears `g.user` even during request context (e.g., make clearing unconditional instead of guarded by `has_request_context()` at lines 155-158). 2. Run `test_mcp_auth_hook_preserves_g_user_in_request_context` in `tests/unit_tests/mcp_service/test_auth_user_resolution.py` (lines 305-323). 3. Current test patches `get_user_from_request` to always return `middleware_user` (lines 317-320); `_setup_user_context()` sets `g.user = middleware_user` afterward (`auth.py` second chunk line 48). 4. `dummy_tool()` reads `g.user.username` (line 311) and still returns `"middleware_user"` (asserted line 323), so accidental pre-clear in request-context mode is not detected. 5. This matters because `mcp_auth_hook` is the common gate on MCP tool/prompt execution through `superset/core/mcp/core_mcp_injection.py` lines 27-31 and 126-130. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** tests/unit_tests/mcp_service/test_auth_user_resolution.py **Line:** 317:319 **Comment:** *Logic Error: This test claims to verify `g.user` is preserved during request context, but the mock always returns `middleware_user` and does not check whether `g.user` was cleared beforehand, so it cannot detect accidental clearing in request-context mode. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=f6bb3a0298cbd997c044da7f1de10e2a74802030f263871ef9ed66b1f2ad2946&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38747&comment_hash=f6bb3a0298cbd997c044da7f1de10e2a74802030f263871ef9ed66b1f2ad2946&reaction=dislike'>👎</a> -- 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]
