From 3d02f373d68d2fae0d4313f7e2abf684a3a1d0fc Mon Sep 17 00:00:00 2001 From: Amin Ghadersohi Date: Mon, 17 Aug 2026 17:55:55 +0000 Subject: [PATCH] fix(mcp): enforce token scopes independently of RBAC --- docs/admin_docs/security/security.mdx | 14 +++++- superset/mcp_service/auth.py | 24 ++++++----- superset/mcp_service/mcp_config.py | 7 ++- .../mcp_service/system/tool/get_schema.py | 8 ++-- .../system/tool/test_get_schema.py | 19 ++++++++ .../unit_tests/mcp_service/test_auth_rbac.py | 43 +++++++++++++++++++ 6 files changed, 96 insertions(+), 19 deletions(-) diff --git a/docs/admin_docs/security/security.mdx b/docs/admin_docs/security/security.mdx index c659d252dfa..ae5ad72be3f 100644 --- a/docs/admin_docs/security/security.mdx +++ b/docs/admin_docs/security/security.mdx @@ -400,7 +400,7 @@ Once enabled, each user manages their own keys from their profile page: 1. Open the user menu (top-right) and click **Info** to navigate to the User Info page 2. Expand the **API Keys** section 3. Click **+ API Key** -4. Enter a name and (optionally) an expiration date +4. Enter a name and optionally select resource scopes 5. Copy the generated token — it is shown only once Only users with the `can_read` and `can_write` permissions on `ApiKey` (granted by default to Admins) can manage API keys. @@ -415,6 +415,18 @@ Authorization: Bearer This works for all REST API endpoints and the MCP server. The request is executed with the permissions of the user who created the key. +#### API Key Scopes + +The creation dialog can restrict an API key to MCP resource actions such as +`superset:dashboard:read` or `superset:chart:write`. A scope is an additional +restriction: it never grants a permission that the creating user does not +already have through Superset RBAC. Write scopes also cover update and delete +operations for that resource; `superset:sqllab:write` covers SQL execution. + +Keys created without scopes retain legacy RBAC-only behavior. The scoped-key +restrictions described here are enforced by the MCP server; regular REST API +routes continue to apply their existing Superset RBAC checks. + #### Use Cases - **CI/CD pipelines** — automated chart/dashboard exports and imports diff --git a/superset/mcp_service/auth.py b/superset/mcp_service/auth.py index 476ca546893..8a237ea075f 100644 --- a/superset/mcp_service/auth.py +++ b/superset/mcp_service/auth.py @@ -153,8 +153,13 @@ def _get_token_scopes() -> set[str] | None: try: access_token = get_access_token() - except Exception: # noqa: BLE001 - no JWT context for this request - return None + except Exception: # noqa: BLE001 - fail closed on token-context errors + logger.exception("Unable to resolve MCP access-token scopes") + # ``None`` means that no scoped credential was presented and enables + # legacy RBAC-only behavior. An empty set instead makes every scope + # check fail, so an unexpected context error cannot erase restrictions + # carried by a credential. + return set() if access_token is None: return None @@ -390,8 +395,13 @@ def check_tool_permission( # noqa: C901 ) return False + method_permission_name = getattr(func, METHOD_PERMISSION_ATTR, "read") + class_permission_name = getattr(func, CLASS_PERMISSION_ATTR, None) + + # Token capabilities and user RBAC are independent restrictions. + # Disabling RBAC must not discard scopes explicitly carried by a key. if not current_app.config.get("MCP_RBAC_ENABLED", True): - return True + return _token_scope_allows(method_permission_name, class_permission_name) if not hasattr(g, "user") or not g.user: if log_denial: @@ -404,8 +414,6 @@ def check_tool_permission( # noqa: C901 ) return False - method_permission_name = getattr(func, METHOD_PERMISSION_ATTR, "read") - class_permission_name = getattr(func, CLASS_PERMISSION_ATTR, None) if not class_permission_name: # No RBAC configured for this tool; allow by default. This is a # supported configuration (a protected tool may intentionally @@ -509,7 +517,7 @@ def is_tool_visible_to_current_user(tool: Any) -> bool: return False if not current_app.config.get("MCP_RBAC_ENABLED", True): - return True + return check_tool_permission(tool_func, log_denial=False) from superset.mcp_service.privacy import ( tool_requires_data_model_metadata_access, @@ -522,10 +530,6 @@ def is_tool_visible_to_current_user(tool: Any) -> bool: ): return False - class_permission_name = getattr(tool_func, CLASS_PERMISSION_ATTR, None) - if not class_permission_name: - return True - return check_tool_permission(tool_func, log_denial=False) except (AttributeError, RuntimeError, ValueError): diff --git a/superset/mcp_service/mcp_config.py b/superset/mcp_service/mcp_config.py index cf085d4b365..6cd9fbb5cb5 100644 --- a/superset/mcp_service/mcp_config.py +++ b/superset/mcp_service/mcp_config.py @@ -653,10 +653,9 @@ def _build_composite_verifier( if api_key_enabled: if required_scopes := app.config.get("MCP_REQUIRED_SCOPES", []): logger.warning( - "MCP_REQUIRED_SCOPES is configured but API key tokens bypass " - "scope enforcement. API key holders gain access regardless of " - "MCP_REQUIRED_SCOPES=%r. Enforce per-key authorization via FAB " - "roles/RBAC instead.", + "MCP_REQUIRED_SCOPES=%r is configured, but API key tokens use " + "the scopes stored on each key instead. Unscoped API keys " + "retain legacy RBAC-only behavior.", required_scopes, ) raw_prefixes: str | Sequence[str] = app.config.get( diff --git a/superset/mcp_service/system/tool/get_schema.py b/superset/mcp_service/system/tool/get_schema.py index 2d6650cfb1e..ca3cff2ad83 100644 --- a/superset/mcp_service/system/tool/get_schema.py +++ b/superset/mcp_service/system/tool/get_schema.py @@ -235,10 +235,10 @@ async def get_schema( from superset import security_manager - if current_app.config.get("MCP_RBAC_ENABLED", True) and not ( - security_manager.can_access("can_read", class_permission) - and _token_scope_allows("read", class_permission) - ): + rbac_allows = not current_app.config.get( + "MCP_RBAC_ENABLED", True + ) or security_manager.can_access("can_read", class_permission) + if not (rbac_allows and _token_scope_allows("read", class_permission)): user_str = getattr(getattr(g, "user", None), "username", None) logger.warning( "get_schema RBAC denied: user=%s type=%s view=%s", diff --git a/tests/unit_tests/mcp_service/system/tool/test_get_schema.py b/tests/unit_tests/mcp_service/system/tool/test_get_schema.py index 30e18f0b2dd..ec238ee4fe0 100644 --- a/tests/unit_tests/mcp_service/system/tool/test_get_schema.py +++ b/tests/unit_tests/mcp_service/system/tool/test_get_schema.py @@ -624,3 +624,22 @@ class TestGetSchemaPermissionMap: ) scope_allows.assert_called_once_with("read", "Chart") + + @pytest.mark.asyncio + async def test_resource_scope_is_enforced_when_rbac_disabled(self, app, mcp_server): + """The RBAC feature flag does not disable credential scopes.""" + with ( + patch.dict(app.config, {"MCP_RBAC_ENABLED": False}), + patch("superset.security_manager.can_access") as can_access, + patch.object( + get_schema_module, "_token_scope_allows", return_value=False + ) as scope_allows, + ): + async with Client(mcp_server) as client: + with pytest.raises(ToolError, match="Permission denied"): + await client.call_tool( + "get_schema", {"request": {"model_type": "chart"}} + ) + + can_access.assert_not_called() + scope_allows.assert_called_once_with("read", "Chart") diff --git a/tests/unit_tests/mcp_service/test_auth_rbac.py b/tests/unit_tests/mcp_service/test_auth_rbac.py index 5f7d2914443..43fd78f414e 100644 --- a/tests/unit_tests/mcp_service/test_auth_rbac.py +++ b/tests/unit_tests/mcp_service/test_auth_rbac.py @@ -183,6 +183,19 @@ def test_check_tool_permission_disabled_via_config(app_context, app) -> None: app.config["MCP_RBAC_ENABLED"] = True +def test_disabled_rbac_still_enforces_token_scopes(app_context, app) -> None: + """Disabling user RBAC does not disable credential restrictions.""" + func = _make_tool_func(class_perm="Chart", method_perm="write") + app.config["MCP_RBAC_ENABLED"] = False + try: + with _patch_token_scopes(["superset:dashboard:read"]): + assert check_tool_permission(func) is False + with _patch_token_scopes(["superset:chart:write"]): + assert check_tool_permission(func) is True + finally: + app.config["MCP_RBAC_ENABLED"] = True + + # -- Permission constants -- @@ -302,6 +315,19 @@ def test_visibility_public_tool_no_class_permission(app_context) -> None: assert is_tool_visible_to_current_user(tool) is True +def test_visibility_hides_permissionless_tool_from_resource_scoped_token( + app_context, +) -> None: + """Permission-less tools require a flat scope in tools/list too.""" + g.user = MagicMock(username="viewer") + tool = _make_mock_tool(fn=_make_tool_func()) + + with _patch_token_scopes(["superset:dashboard:read"]): + assert is_tool_visible_to_current_user(tool) is False + with _patch_token_scopes(["superset:read"]): + assert is_tool_visible_to_current_user(tool) is True + + def test_visibility_allowed_tool(app_context) -> None: """Tools where security_manager grants access are visible.""" g.user = MagicMock(username="admin") @@ -444,6 +470,23 @@ def test_scope_falls_back_to_rbac_when_no_jwt_context(app_context) -> None: assert result is True +def test_scope_context_error_fails_closed(app_context) -> None: + """An unexpected token lookup failure cannot erase token restrictions.""" + g.user = MagicMock(username="editor") + func = _make_tool_func(class_perm="Chart", method_perm="read") + + mock_sm = MagicMock() + mock_sm.can_access = MagicMock(return_value=True) + with ( + patch("superset.mcp_service.auth.security_manager", mock_sm), + patch( + "fastmcp.server.dependencies.get_access_token", + side_effect=TypeError("invalid token context"), + ), + ): + assert check_tool_permission(func) is False + + def test_scope_read_denied_when_token_lacks_read_scope(app_context) -> None: """A read tool is denied when the token only carries an unrelated scope.""" g.user = MagicMock(username="viewer")