gabotorresruiz commented on code in PR #45984:
URL: https://github.com/apache/superset/pull/45984#discussion_r4460653704
##########
superset/mcp_service/auth.py:
##########
@@ -185,11 +185,16 @@ def _get_token_scopes() -> set[str] | None:
if access_token is None:
return None
- scopes = getattr(access_token, "scopes", None)
- if not scopes:
+ scopes_from_token = getattr(access_token, "scopes", None)
+ if not scopes_from_token:
# Token present but no scopes advertised: do NOT enforce scope checks.
return None
- return {str(s) for s in scopes}
+ ignored_scopes = current_app.config.get("MCP_IGNORED_SCOPES", {})
+ scopes = {str(s) for s in scopes_from_token if str(s) not in
ignored_scopes}
Review Comment:
Not a blocker, but this is the one line that decides whether scope
enforcement runs at all, so I would really like to see it pinned in
`tests/unit_tests/mcp_service/test_auth_rbac.py` before merge. Three flat tests
using the existing `_patch_token_scopes` helper cover it; the first fails on
master and passes here, the other two pin that dropping `openid` never loosens
a token that also carries a real scope, and that the default stays unchanged:
```python
def test_ignored_scopes_restore_rbac_only_when_nothing_remains(app_context,
app) -> None:
"""A token whose only scopes are all ignored is treated as scope-less."""
g.user = MagicMock(username="editor")
func = _make_tool_func(class_perm="Chart", method_perm="write")
mock_sm = MagicMock()
mock_sm.can_access = MagicMock(return_value=True)
app.config["MCP_IGNORED_SCOPES"] = {"openid", "profile", "email",
"offline_access"}
try:
with (
patch("superset.mcp_service.auth.security_manager", mock_sm),
_patch_token_scopes(["openid", "profile"]),
):
assert check_tool_permission(func) is True
with _patch_token_scopes(["openid"]):
assert check_tool_permission(_make_tool_func()) is True
finally:
app.config.pop("MCP_IGNORED_SCOPES", None)
def test_ignored_scopes_keep_remaining_scope_enforced(app_context, app) ->
None:
"""Dropping ``openid`` must not loosen a token that also carries a real
scope."""
g.user = MagicMock(username="editor")
func = _make_tool_func(class_perm="Chart", method_perm="write")
mock_sm = MagicMock()
mock_sm.can_access = MagicMock(return_value=True)
app.config["MCP_IGNORED_SCOPES"] = {"openid"}
try:
with (
patch("superset.mcp_service.auth.security_manager", mock_sm),
_patch_token_scopes(["openid", "superset:read"]),
):
assert check_tool_permission(func) is False
finally:
app.config.pop("MCP_IGNORED_SCOPES", None)
def test_ignored_scopes_default_leaves_scoped_tokens_enforced(app_context)
-> None:
"""With the default (empty) ignored set, an identity-only token still
denies."""
g.user = MagicMock(username="editor")
with _patch_token_scopes(["openid"]):
assert check_tool_permission(_make_tool_func()) is False
```
##########
superset/mcp_service/auth.py:
##########
@@ -185,11 +185,16 @@ def _get_token_scopes() -> set[str] | None:
if access_token is None:
return None
- scopes = getattr(access_token, "scopes", None)
- if not scopes:
+ scopes_from_token = getattr(access_token, "scopes", None)
+ if not scopes_from_token:
# Token present but no scopes advertised: do NOT enforce scope checks.
return None
- return {str(s) for s in scopes}
+ ignored_scopes = current_app.config.get("MCP_IGNORED_SCOPES", {})
+ scopes = {str(s) for s in scopes_from_token if str(s) not in
ignored_scopes}
+ if not scopes:
+ # Token where all scopes advertised are ignored: do NOT enforce scope
checks.
+ return None
Review Comment:
Just to set expectations for the testing instructions: this branch only
makes the `Scope denied for permission-less tool ...` line go away. The `is
permission-protected but declares no class_permission_name` warning is emitted
unconditionally once per tool whenever RBAC is on (in `check_tool_permission`,
right before the scope check), so it still shows up once per tool here. I
verified it with a `scope=openid` token against `superset mcp run`. Might be
worth trimming the PR description so nobody reads that remaining line as the
option not working.
--
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]
