fix(mcp): correct method name in API key auth (extract_api_key_from_request)

[aminghadersohi]

SUMMARY

Fixes a method name mismatch in superset/mcp_service/auth.py that caused MCP API key authentication to silently fail.

_resolve_user_from_api_key() referenced sm._extract_api_key_from_request (private, underscore prefix), but FAB defines it as sm.extract_api_key_from_request (public, no underscore). The hasattr check always returned False, so the function returned None immediately and MCP fell through to MCP_DEV_USERNAME / g.user instead of authenticating via API key.

Changes:

• superset/mcp_service/auth.py: Rename _extract_api_key_from_request → extract_api_key_from_request in the hasattr guard and the call site (2 lines)
• tests/unit_tests/mcp_service/test_auth_api_key.py: Update all existing mock references to the correct method name, and add a regression test (test_security_manager_has_expected_api_key_methods) that asserts both method names referenced in auth.py actually exist on the real SecurityManager object — so CI catches future name mismatches instead of silently failing at runtime

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — auth bug fix with no UI changes.

TESTING INSTRUCTIONS

1. Run pytest tests/unit_tests/mcp_service/test_auth_api_key.py — all tests should pass
2. To manually verify: create an API key via /api/v1/security/api_keys/, then call an MCP tool with Authorization: Bearer sst_<key> — should authenticate correctly instead of falling through to dev user

ADDITIONAL INFORMATION

• Has associated issue: SC-99414 (Shortcut)
• Required feature flags:
• Changes UI
• Includes DB Migration
• Introduces new feature or API
• Removes existing feature or API

Relates to #37973 (the PR that added API key auth — this fixes the MCP integration that failed QA 0/6).

🤖 Generated with Claude Code

[bito-code-review]

Code Review Agent Run #4ddf1b

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: 6e34690..6e34690

  • superset/mcp_service/auth.py
  • tests/unit_tests/mcp_service/test_auth_api_key.py
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers a full AI review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[codecov]

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.98%. Comparing base (4bdc8d4) to head (6e34690).
⚠️ Report is 104 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/mcp_service/auth.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #39437      +/-   ##
==========================================
- Coverage   64.49%   63.98%   -0.52%     
==========================================
  Files        2557     2562       +5     
  Lines      133097   137608    +4511     
  Branches    30910    31981    +1071     
==========================================
+ Hits        85846    88042    +2196     
- Misses      45761    47970    +2209     
- Partials     1490     1596     +106     

Flag	Coverage Δ
hive	39.51% <0.00%> (-0.36%)	⬇️
mysql	59.62% <0.00%> (-0.88%)	⬇️
postgres	59.71% <0.00%> (-0.88%)	⬇️
presto	41.36% <0.00%> (-0.30%)	⬇️
python	61.18% <0.00%> (-0.98%)	⬇️
sqlite	59.36% <0.00%> (-0.86%)	⬇️
unit	100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[eschutho]

Review: fix(mcp): correct method name in API key auth

Verdict: Approve with minor notes. The fix is correct, minimal, and addresses a real auth-bypass bug. The method name extract_api_key_from_request is confirmed correct against the installed FAB 5.2.1 (manager.py:2525, @staticmethod; also called identically in FAB's own decorators.py:105). The regression test concept is excellent. A few things worth noting below.

---

What works well

• Correct fix, minimal diff. Exactly the two call sites in _resolve_user_from_api_key needed to change — nothing more.
• All mock references updated consistently. Six test cases updated across test_auth_api_key.py; none missed.
• Comment improved accurately. Removing "internal" from the comment is right — extract_api_key_from_request is a public @staticmethod on BaseSecurityManager, not an internal method.
• Regression test idea is solid. Checking that method names referenced in auth.py exist on the real SecurityManager is exactly the right kind of guard against future silent failures.
• Security impact was severe. hasattr(sm, "_extract_api_key_from_request") always returned False, so API key auth was completely bypassed on every request — valid keys couldn't authenticate as themselves, and requests fell through to MCP_DEV_USERNAME (potentially a high-privilege admin user) or stale g.user from middleware. This fix closes that gap.

---

Issues

1. [Minor] Regression test may not exercise the fully-initialized SecurityManager

test_security_manager_has_expected_api_key_methods has no app fixture and does not push an app context:

# tests/unit_tests/mcp_service/test_auth_api_key.py (new test, bottom of file)
def test_security_manager_has_expected_api_key_methods() -> None:
    from superset import security_manager
    sm = security_manager
    assert hasattr(sm, "extract_api_key_from_request"), ...
    assert hasattr(sm, "validate_api_key"), ...

In Superset, security_manager is a module-level proxy that may not be bound to the AppBuilder-initialized instance without an active app context. The hasattr check might pass trivially against the proxy class itself rather than the real initialized SM. Adding the app fixture (as every other test in this file does) ensures this test exercises the actual runtime object:

def test_security_manager_has_expected_api_key_methods(app) -> None:
    with app.app_context():
        from superset import security_manager
        sm = security_manager
        ...

2. [Nit] Internal ticket reference in docstring

"""...This catches future renames before they silently break API key auth
at runtime (SC-99414: _extract_api_key_from_request vs
extract_api_key_from_request)."""\n```

`SC-99414` is an internal ticket that's opaque to external contributors. Replace with a reference to this GitHub PR number (or the issue it fixes) so the history is navigable from the open-source repo.

**3. [Nit] Pre-existing gap in test coverage (not introduced here, but worth noting)**

There is no test for the path where `extract_api_key_from_request()` returns `None` (i.e., the Bearer token is present but doesn't match any `FAB_API_KEY_PREFIXES`). The code correctly handles it with `if api_key_string is None: return None`, but a test covering that path would make the suite complete.

---

### Questions

1. Is `extract_api_key_from_request` available in all FAB versions that Superset supports, or only from a certain version? The existing `hasattr` guard implies it might not always be present — it would be helpful to document which minimum FAB version introduced this method (or point to the FAB release notes).

2. Was `FAB_API_KEY_ENABLED` ever set to `True` in any Superset deployment configs, or was this essentially dead code until now? Knowing the blast radius helps assess whether a CHANGELOG/UPDATING.md entry is warranted.

---

Overall this is a clear, correct fix for a silent security regression. The two minor points above are optional improvements; they shouldn't block merge.

[eschutho]

Review: fix(mcp): correct method name in API key auth

Verdict: Approve with minor notes. The fix is correct, minimal, and addresses a real auth-bypass bug. The method name extract_api_key_from_request is confirmed correct against the installed FAB 5.2.1 (manager.py:2525, defined as @staticmethod; also called identically in FAB's own decorators.py:105). A few things worth noting below.

---

What works well

• Correct fix, minimal diff. Exactly the two call sites in _resolve_user_from_api_key needed to change — nothing more.
• All mock references updated consistently. Six test cases in test_auth_api_key.py updated; none missed.
• Comment improved accurately. Removing "internal" from the comment is right — extract_api_key_from_request is a public @staticmethod on BaseSecurityManager, not an internal method.
• Regression test idea is solid. Checking that method names referenced in auth.py exist on the real SecurityManager is exactly the right guard against future silent failures.
• Security impact was severe. hasattr(sm, "_extract_api_key_from_request") always returned False, so API key auth was completely bypassed on every request — valid keys couldn't authenticate as themselves, and requests fell through to MCP_DEV_USERNAME (potentially a high-privilege admin user) or stale g.user from middleware. This fix closes that gap.

---

Issues

1. [Minor] Regression test may not exercise the fully-initialized SecurityManager

test_security_manager_has_expected_api_key_methods has no app fixture and does not push an app context:

# tests/unit_tests/mcp_service/test_auth_api_key.py (new test, bottom of file)
def test_security_manager_has_expected_api_key_methods() -> None:
    from superset import security_manager
    sm = security_manager
    assert hasattr(sm, "extract_api_key_from_request"), ...

In Superset, security_manager is a module-level proxy that may not be bound to the AppBuilder-initialized instance without an active app context. The hasattr check might pass trivially against the proxy class rather than the real initialized SM. Adding the app fixture (as every other test in this file does) and pushing an app context ensures this test exercises the actual runtime object.

2. [Nit] Internal ticket reference in docstring

The SC-99414 reference in the new test docstring is an internal ticket opaque to external contributors. Replace with a GitHub PR or issue number so the history is navigable from the public repo.

3. [Nit] Pre-existing gap in test coverage (not introduced here)

There is no test for the path where extract_api_key_from_request() returns None (Bearer token present but doesn't match any FAB_API_KEY_PREFIXES). The code handles it correctly; a test for that path would complete the suite.

---

Questions

1. Is extract_api_key_from_request available in all FAB versions Superset supports, or only from a certain version? The hasattr guard implies it may not always be present — documenting the minimum FAB version that introduced this method would clarify the guard's intent.

2. Was FAB_API_KEY_ENABLED=True used in any real Superset deployments before this fix? Knowing the blast radius helps assess whether a CHANGELOG/UPDATING.md entry is warranted.

---

Overall this is a clear, correct fix for a silent security regression. The minor points above are optional improvements and should not block merge.

[aminghadersohi]

Thanks for the thorough review @eschutho! All three items addressed in the latest commit:

Issue 1 (regression test needs app context): Updated test_security_manager_has_expected_api_key_methods to accept the app fixture and wrap assertions in app.app_context() so it exercises the real initialized SecurityManager, not the module-level proxy.

Issue 2 (SC-99414 internal reference): Replaced with a reference to this PR (PR #39437) so it's navigable from the public repo.

Issue 3 (missing test for non-matching prefix): Added test_non_matching_bearer_token_skips_api_key_auth — covers the path where a Bearer token is present but extract_api_key_from_request() returns None (doesn't match FAB_API_KEY_PREFIXES). Asserts that validate_api_key is never called in that case.

Q1 (minimum FAB version): extract_api_key_from_request and validate_api_key were introduced in FAB 5.2.x. Superset requires flask-appbuilder>=5.2.1,<6.0.0 (pyproject.toml), so the methods are available in all supported versions. The hasattr guard is defensive against forks or custom SecurityManager subclasses that might omit these methods.

Q2 (blast radius): FAB_API_KEY_ENABLED defaults to False and the feature was only added in #37973. No existing deployments would have been using this — API key auth was effectively dead code because the method name mismatch caused it to silently skip on every request. No CHANGELOG/UPDATING.md entry needed since the feature never worked in a released version.