rusackas commented on code in PR #40631:
URL: https://github.com/apache/superset/pull/40631#discussion_r3338055064
##########
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:
Fixed — `_reject_if_inactive` now checks `getattr(user, 'is_active', True)
and getattr(user, 'active', True)`. A user is rejected if either flag is False,
so it works across both old FAB (`active`) and Flask-Login / newer FAB
(`is_active`) deployments.
##########
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:
Updated the test to set both `is_active = False` and `active = False` so it
validates the check regardless of which attribute the implementation reads.
##########
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:
Same here — set both `is_active = True` and `active = True` on the
active-user mock for cross-version robustness.
--
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]
