Copilot commented on code in PR #40631:
URL: https://github.com/apache/superset/pull/40631#discussion_r3337826115
##########
tests/unit_tests/mcp_service/test_auth_user_resolution.py:
##########
@@ -535,3 +535,41 @@ def test_setup_user_context_propagates_valueerror(app) ->
None:
_setup_user_context()
# g.user should be cleared after ValueError (no misleading audit)
assert not hasattr(g, "user") or g.user is None
+
+
+def test_setup_user_context_rejects_disabled_user(app) -> None:
+ """A deactivated account must be denied even with a valid token.
+
+ The MCP auth path does not go through Flask-Login's is_active check, so
+ this guards that a disabled user (active=False) cannot authenticate.
+ """
+ from superset.mcp_service.auth import _setup_user_context
+
+ disabled_user = _make_mock_user("disabled_user")
+ disabled_user.is_active = False
+
Review Comment:
The test only sets `is_active`, but FAB has historically used `active` as
the canonical flag (and some user-like objects may expose only `active`).
Setting both here makes the test resilient across versions and matches the
docstring’s `active=False` wording.
##########
superset/mcp_service/auth.py:
##########
@@ -430,6 +430,17 @@ def check_chart_data_access(chart: Any) ->
"DatasetValidationResult":
return validate_chart_dataset(chart, check_access=True)
+def _reject_if_inactive(user: User | None) -> None:
+ """Raise ``ValueError`` if the resolved user account is deactivated.
+
+ A still-valid JWT or API key must not grant MCP access to a user whose
+ account has been disabled. This mirrors Flask-Login's ``is_active`` check
+ for web sessions, which the MCP auth path does not otherwise go through.
+ """
+ if user is not None and not getattr(user, "is_active", True):
+ raise ValueError("User account is disabled")
Review Comment:
_reject_if_inactive only inspects the `is_active` attribute.
Flask-AppBuilder historically used `active` (and Superset’s GuestUser
explicitly keeps both for backwards compatibility), so on deployments where the
resolved User exposes only `active`, this check would incorrectly treat
deactivated accounts as active and allow MCP access.
##########
tests/unit_tests/mcp_service/test_auth_user_resolution.py:
##########
@@ -535,3 +535,41 @@ def test_setup_user_context_propagates_valueerror(app) ->
None:
_setup_user_context()
# g.user should be cleared after ValueError (no misleading audit)
assert not hasattr(g, "user") or g.user is None
+
+
+def test_setup_user_context_rejects_disabled_user(app) -> None:
+ """A deactivated account must be denied even with a valid token.
+
+ The MCP auth path does not go through Flask-Login's is_active check, so
+ this guards that a disabled user (active=False) cannot authenticate.
+ """
+ from superset.mcp_service.auth import _setup_user_context
+
+ disabled_user = _make_mock_user("disabled_user")
+ disabled_user.is_active = False
+
+ with app.test_request_context():
+ with patch(
+ "superset.mcp_service.auth.get_user_from_request",
+ return_value=disabled_user,
+ ):
+ with pytest.raises(ValueError, match="disabled"):
+ _setup_user_context()
+ assert not hasattr(g, "user") or g.user is None
+
+
+def test_setup_user_context_allows_active_user(app) -> None:
+ """An active account authenticates normally."""
+ from superset.mcp_service.auth import _setup_user_context
+
+ active_user = _make_mock_user("active_user")
+ active_user.is_active = True
+
Review Comment:
For the same cross-version robustness as the disabled-user test, set both
`is_active` and `active` on the mock user so this test continues to validate
behavior regardless of which attribute the implementation checks.
--
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]
