(superset) 16/30: fix(mcp): fix stale patch target in auth tests and update stale docstring

[mi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

aminghadersohi pushed a commit to branch oss-39604
in repository https://gitbox.apache.org/repos/asf/superset.git

commit dc17712916bf319ca4e5f33dc12b2d8df02218a9
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Thu May 14 19:05:42 2026 +0000

    fix(mcp): fix stale patch target in auth tests and update stale docstring
    
    - Remove mock_sm.find_user_with_relationships.return_value = None from
      _mock_sm_ctx: load_user_with_relationships delegates to the global
      security_manager (not app.appbuilder.sm), so setting it on mock_sm had
      no effect and broke MagicMock(spec=[]) tests.
    - Add _patch_load_user_not_found() helper that patches
      superset.mcp_service.auth.load_user_with_relationships directly.
    - Apply it to the 3 JWT-path tests that expect ValueError("not found"):
      test_jwt_access_token_skips_api_key_auth,
      test_namespaced_claim_without_api_key_client_id_is_ignored,
      test_unnamespaced_passthrough_claim_does_not_trigger_api_key_path.
    
    Co-Authored-By: Claude Sonnet 4.6 <no...@anthropic.com>
---
 tests/unit_tests/mcp_service/test_auth_api_key.py | 43 ++++++++++++-----------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/tests/unit_tests/mcp_service/test_auth_api_key.py b/tests/unit_tests/mcp_service/test_auth_api_key.py
index e4279ffcd9d..afe5a53ac5b 100644
--- a/tests/unit_tests/mcp_service/test_auth_api_key.py
+++ b/tests/unit_tests/mcp_service/test_auth_api_key.py
@@ -86,14 +86,7 @@ def _disable_api_keys(app: SupersetApp) -> Generator[None, None, None]:
 
 @contextmanager
 def _mock_sm_ctx(app: SupersetApp, mock_sm: MagicMock):
-    """Push an app context with g.user cleared and appbuilder.sm mocked.
-
-    Defaults find_user_with_relationships to None so JWT/dev-user lookups
-    that hit the SM (via load_user_with_relationships) behave as "user not
-    found" without a real DB, matching the pre-refactor db.session behavior.
-    Tests that need a specific return value should set it on mock_sm directly.
-    """
-    mock_sm.find_user_with_relationships.return_value = None
+    """Push an app context with g.user cleared and appbuilder.sm mocked."""
     with app.app_context():
         g.user = None
         app.appbuilder = MagicMock()
@@ -101,6 +94,19 @@ def _mock_sm_ctx(app: SupersetApp, mock_sm: MagicMock):
         yield
 
 
+def _patch_load_user_not_found():
+    """Patch load_user_with_relationships to return None (user not found).
+
+    load_user_with_relationships delegates to the global security_manager
+    (not app.appbuilder.sm), so tests that need the JWT path to raise
+    ValueError("not found") must patch it directly at the module level.
+    """
+    return patch(
+        "superset.mcp_service.auth.load_user_with_relationships",
+        return_value=None,
+    )
+
+
 # -- Valid API key -> user loaded --
 
 
@@ -255,10 +261,9 @@ def test_jwt_access_token_skips_api_key_auth(app: SupersetApp) -> None:
     jwt_access_token.claims = {"sub": "alice"}
 
     with _mock_sm_ctx(app, mock_sm):
-        with _patch_access_token(jwt_access_token):
-            # _resolve_user_from_jwt_context will try to resolve the user
-            # from the JWT claims and (in this isolated unit-test setup)
-            # raise ValueError because the username is not a real user.
+        with _patch_access_token(jwt_access_token), _patch_load_user_not_found():
+            # _resolve_user_from_jwt_context resolves "alice" from JWT claims
+            # and raises ValueError because the username is not a real user.
             # We assert that _resolve_user_from_api_key did NOT short-circuit
             # to the API key path.
             with pytest.raises(ValueError, match="not found"):
@@ -302,9 +307,9 @@ def test_namespaced_claim_without_api_key_client_id_is_ignored(
     rogue_token.claims = {API_KEY_PASSTHROUGH_CLAIM: True, "sub": "alice"}
 
     with _mock_sm_ctx(app, mock_sm):
-        with _patch_access_token(rogue_token):
-            # JWT path tries to resolve user "alice" from DB and raises
-            # ValueError in this isolated unit-test setup.
+        with _patch_access_token(rogue_token), _patch_load_user_not_found():
+            # JWT path resolves "alice" from claims and raises ValueError
+            # because no such user exists.
             # validate_api_key must NOT be called — the rogue claim was ignored.
             with pytest.raises(ValueError, match="not found"):
                 get_user_from_request()
@@ -330,11 +335,9 @@ def test_unnamespaced_passthrough_claim_does_not_trigger_api_key_path(
     rogue_token.claims = {"_api_key_passthrough": True, "sub": "alice"}
 
     with _mock_sm_ctx(app, mock_sm):
-        with _patch_access_token(rogue_token):
-            # JWT path tries to resolve user "alice" from DB and (in this
-            # isolated unit-test setup) raises ValueError. The assertion
-            # below confirms validate_api_key was never called — i.e., the
-            # rogue claim did NOT divert into _resolve_user_from_api_key.
+        with _patch_access_token(rogue_token), _patch_load_user_not_found():
+            # JWT path resolves "alice" from claims and raises ValueError.
+            # validate_api_key must NOT be called — the rogue claim was ignored.
             with pytest.raises(ValueError, match="not found"):
                 get_user_from_request()