aminghadersohi commented on code in PR #44146:
URL: https://github.com/apache/superset/pull/44146#discussion_r4059682519
##########
docs/admin_docs/configuration/mcp-server.mdx:
##########
@@ -1074,3 +1074,68 @@ once, even when multiple dimensions are missing.
The tool does not rewrite dots into SQL path separators: quoting and
nested-field
support depend on the dataset's database dialect, and dots can also be literal
characters in column names.
+
+## Dataset discovery and routing
+
+`list_datasets` searches table name, **description**, schema, and SQL using
+escaped, case-insensitive substring matching. Write descriptions with business
+terminology to make differently named datasets discoverable. A complete UUID
+passed as `search` is treated as an exact UUID filter; an explicit `uuid`
filter
+can also look up one or more datasets by UUID. Matching
+datasets are candidates, ordered by the requested sort (last modified by
default), not
+ranked recommendations. Compare descriptions and metrics, present alternatives
+when ambiguous, and clarify before querying. No matches does not prove that
data
+does not exist; search only covers accessible datasets and the configured
scope.
+
+`query_dataset` returns `dataset_id` and `dataset_name`; `get_table`
additionally
+returns `source` and the corresponding dataset or external-view identity. Cite
+these fields in answers rather than guessing the source from the query text.
+Neither tool selects an alternative dataset automatically.
+
+### Optional per-role dataset scope
+
+Set `MCP_DATASET_ROLE_ALLOWLIST` in `superset_config.py` to constrain MCP to a
+curated set of registered datasets:
+
+```python
+MCP_DATASET_ROLE_ALLOWLIST = {
+ "Finance Readers": ["00000000-0000-0000-0000-000000000001"],
+ "Operations Readers": ["00000000-0000-0000-0000-000000000002"],
+}
+```
+
+Obtain dataset UUIDs from `get_dataset_info` (or request `uuid` in
+`list_datasets.select_columns`). Use UUIDs, not names or numeric IDs, in
config.
+Effective roles, including group roles, contribute the **union** of their
lists.
+That union is **intersected with existing dataset access**. Unconfigured roles
+contribute nothing; Admin has no routing exemption. `None` (the default)
disables
+this feature; `{}` allows no datasets. Invalid entries cause a configuration
+error at startup rather than silently disabling the restriction.
+
+Scoped mode supports the dataset tools that name a registered dataset —
+`list_datasets`, `get_dataset_info`, `query_dataset`, and, with a built-in
+dataset, `get_table`, `list_metrics`, `get_compatible_dimensions` and
+`get_compatible_metrics` — plus `health_check` and `get_schema`.
+Discovery filters before counting and pagination. Other tools refuse in this
Review Comment:
Verified against the rebased code: valid documentation finding, closed by
55e1af92718d98a0f0ecbf2d23c2d3105d1ffbfb (the rebased equivalent of a0f7522).
The docs explicitly limit the refusal claim to authenticated tools and name
`generate_bug_report` / `protect=False` / `ALLOWED_UNPROTECTED`.
`core_mcp_injection.py` uses `if protect: ... mcp_auth_hook(...)` and `else:
wrapped_func = func`, confirming the exception. The public-tool regression test
passes. Follow-up 5fa47d9bcb5975f9d3455e476b892ff4dab5c88b also documents the
embedded-guest implication noted in the approval body. Resolving; I am
re-requesting your review.
##########
docs/admin_docs/configuration/mcp-server.mdx:
##########
@@ -1180,3 +1180,73 @@ once, even when multiple dimensions are missing.
The tool does not rewrite dots into SQL path separators: quoting and
nested-field
support depend on the dataset's database dialect, and dots can also be literal
characters in column names.
+
+## Dataset discovery and routing
+
+`list_datasets` searches table name, **description**, schema, and SQL using
+escaped, case-insensitive substring matching. Write descriptions with business
+terminology to make differently named datasets discoverable. A complete UUID
+passed as `search` is treated as an exact UUID filter; an explicit `uuid`
filter
+can also look up one or more datasets by UUID. Matching
+datasets are candidates, ordered by the requested sort (last modified by
default), not
+ranked recommendations. Compare descriptions and metrics, present alternatives
+when ambiguous, and clarify before querying. No matches does not prove that
data
+does not exist; search only covers accessible datasets and the configured
scope.
+
+`query_dataset` returns `dataset_id` and `dataset_name`; `get_table`
additionally
+returns `source` and the corresponding dataset or external-view identity. Cite
+these fields in answers rather than guessing the source from the query text.
+Neither tool selects an alternative dataset automatically.
+
+### Optional per-role dataset scope
+
+Set `MCP_DATASET_ROLE_ALLOWLIST` in `superset_config.py` to constrain MCP to a
+curated set of registered datasets:
+
+```python
+MCP_DATASET_ROLE_ALLOWLIST = {
+ "Finance Readers": ["00000000-0000-0000-0000-000000000001"],
+ "Operations Readers": ["00000000-0000-0000-0000-000000000002"],
+}
+```
+
+Obtain dataset UUIDs from `get_dataset_info` (or request `uuid` in
+`list_datasets.select_columns`). Use UUIDs, not names or numeric IDs, in
config.
+Effective roles, including group roles, contribute the **union** of their
lists.
+That union is **intersected with existing dataset access**. Unconfigured roles
+contribute nothing; Admin has no routing exemption. `None` (the default)
disables
+this feature; `{}` allows no datasets. Invalid entries cause a configuration
+error at startup rather than silently disabling the restriction.
+
+Scoped mode supports the dataset tools that name a registered dataset —
+`list_datasets`, `get_dataset_info`, `query_dataset`, and, with a built-in
+dataset, `get_table`, `list_metrics`, `get_compatible_dimensions` and
+`get_compatible_metrics` — plus `health_check` and `get_schema`.
+Discovery filters before counting and pagination. Every other **authenticated**
+tool refuses in this mode, including SQL Lab, chart/dashboard previews and
data,
+cached query results, external semantic sources, mutations, and extension
tools.
+The one exception is `generate_bug_report`, which is deliberately public
+(`protect=False`, listed in `ALLOWED_UNPROTECTED`) so diagnostics stay
available
+even when authentication is broken. Scoped mode is enforced in the
+authentication wrapper, so that tool keeps working; it reports environment and
+user-supplied diagnostics and reads no dataset rows. These paths cannot
+all be attributed reliably to registered dataset UUIDs; scoped mode
deliberately
+refuses them instead of attempting SQL lineage inference. Unsupported tools
stay
+listed and refuse when called, so the assistant reads an explanation it can
pass
+on rather than finding the tool missing. Prompts and resources are unaffected:
+they expose schema and instance metadata, not dataset rows. This restriction
+applies to every caller of the MCP tools, not just a particular chat client.
+
+An out-of-scope request returns an explicit refusal. The assistant must explain
+that limitation, never silently substitute an allowed but different dataset.
+This feature controls routing and usability, **not authorization or SQL table
+access**. Existing dataset permissions, query validation, and row-level
security
+remain mandatory and unchanged. A registered virtual dataset may itself query
+multiple physical tables; this configuration selects registered dataset
objects,
+not their underlying SQL dependencies. It does not restrict the Superset
UI/API.
+
+Description search adds one text predicate to an existing catalog substring
+scan, without joins or additional queries, and replaces the `uuid` predicate
+that could never match. It is not an indexed full-text search: large catalogs
Review Comment:
Verified: valid docs finding, closed by
2ad5158f3f87c1b3ec10d662f55c9d914c5ab832 (rebased equivalent of 983158a). The
paragraph reads: `replaces the uuid substring predicate, whose behavior varied
by database`. This matches `schema_discovery.py` and the portable exact-UUID
search path; the affected dataset tests pass. No routing behavior change was
needed for this finding. Resolving without re-requesting your already-approved
review.
##########
superset/mcp_service/dataset_scope.py:
##########
@@ -0,0 +1,223 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Optional MCP dataset routing constraints, independent of authorization."""
+
+import inspect
+from collections.abc import Mapping
+from typing import Any, TYPE_CHECKING
+from uuid import UUID
+
+from fastmcp.exceptions import ToolError
+from flask import current_app, g, has_app_context
+from flask_appbuilder.models.filters import BaseFilter
+
+from superset import security_manager
+
+if TYPE_CHECKING:
+ from sqlalchemy.orm.query import Query
+
+CONFIG_KEY = "MCP_DATASET_ROLE_ALLOWLIST"
+
+_NO_SUBSTITUTE = (
+ "No query was run. Do not substitute another dataset; explain the scope "
+ "limitation and ask an administrator to review the routing configuration."
+)
+
+# Distinct refusals so a misrouted request is distinguishable from a tool that
+# scoped mode does not support at all — both for callers and for regression
+# tests, which would otherwise pass against an implementation that refuses
+# unconditionally.
+UNSUPPORTED_TOOL_ERROR = (
+ "This tool is unavailable while MCP is running with a configured dataset "
+ f"scope, because its results cannot be attributed to a specific registered
"
+ f"dataset. {_NO_SUBSTITUTE}"
+)
+NO_DATASET_IDENTITY_ERROR = (
+ "This request does not identify a registered dataset, which MCP requires "
+ f"while running with a configured dataset scope. {_NO_SUBSTITUTE}"
+)
+OUT_OF_SCOPE_ERROR = (
+ "The requested dataset is outside the configured MCP dataset scope. "
+ f"{_NO_SUBSTITUTE}"
+)
+
+# Only these tools can operate in dataset-scoped mode. Other paths can read
data
+# through SQL, cached results, screenshots, or external semantic sources
without
+# a registered dataset identity. Refuse them rather than guess at their
lineage.
+#
+# This deliberately gates execution only, not ``tools/list`` visibility. Tool
+# listings are assembled by the tool-search transform, which synthesizes its
own
+# meta tools; filtering that listing on this set would hide the very tools a
+# client needs to reach the scoped ones. A refusal the model can read is a
+# better failure than a tool surface that silently disappears.
+SCOPED_TOOLS = frozenset(
+ {
+ "health_check",
+ "get_schema",
+ "list_datasets",
+ "get_dataset_info",
+ "query_dataset",
+ "get_table",
+ # The metric/dimension discovery tools get_table's own documented
+ # workflow starts with. They name a dataset, so they can be scoped, and
+ # refusing them would block the allowed tool they lead into.
+ "list_metrics",
+ "get_compatible_dimensions",
+ "get_compatible_metrics",
+ }
+)
+
+# Tools whose request names a single dataset, mapped to the field that names
it.
+DATASET_IDENTIFIER_FIELDS = {
+ "get_dataset_info": "identifier",
+ "query_dataset": "dataset_id",
+ "get_table": "dataset_id",
+ "list_metrics": "dataset_id",
+ "get_compatible_dimensions": "dataset_id",
+ "get_compatible_metrics": "dataset_id",
+}
+
+
+class MCPDatasetScopeError(ToolError):
+ """Raised when a call is refused by, or the config of, the routing
allowlist.
+
+ Subclasses ``ToolError`` so the explanation reaches the caller verbatim
+ instead of being flattened into a generic internal error. Surfacing a
+ configuration problem rather than quietly ignoring the setting keeps a typo
+ from silently restoring the unrestricted tool surface an operator opted out
+ of.
+ """
+
+
+class DatasetScopeFilter(BaseFilter): # pylint: disable=too-few-public-methods
+ """Restrict a dataset query to the caller's routing allowlist.
+
+ Applied as a custom filter rather than a caller-visible column operator so
+ the resolved allowlist — which may name datasets the caller cannot access —
+ is not echoed back in the ``filters_applied`` section of a list response.
+ """
+
+ name = "MCP dataset scope"
+ arg_name = "mcp_dataset_scope"
+
+ def apply(self, query: "Query", value: frozenset[UUID]) -> "Query":
+ from superset.connectors.sqla.models import SqlaTable
+
+ return query.filter(SqlaTable.uuid.in_(value))
+
+
+def parse_dataset_role_allowlist(config: Any) -> dict[str, set[UUID]] | None:
+ """Validate the allowlist mapping; None means routing constraints are off.
+
+ Split out from scope resolution so a deployment can fail at startup on a
+ malformed mapping rather than on every subsequent tool call.
+ """
+ if config is None:
+ return None
+ if not isinstance(config, dict):
+ raise MCPDatasetScopeError(
+ f"{CONFIG_KEY} must map role names to lists of dataset UUIDs."
+ )
+ normalized: dict[str, set[UUID]] = {}
+ try:
+ for role, identifiers in config.items():
+ if not isinstance(role, str) or not isinstance(identifiers, (list,
tuple)):
+ raise ValueError("Expected role names and UUID lists")
+ normalized[role] = {UUID(str(identifier)) for identifier in
identifiers}
Review Comment:
Final verification: the `UUID("True")` premise is invalid; I reproduced
`ValueError: badly formed hexadecimal UUID string` for `UUID(str(True))`,
`UUID(str(123))`, `UUID(str(None))`, and the suggested `Uuid('...')` string.
The type-validation improvement was valid and is closed by
55e1af92718d98a0f0ecbf2d23c2d3105d1ffbfb (rebased a0f7522). The actual code is:
```python
if isinstance(identifier, UUID):
return identifier
if not isinstance(identifier, str):
raise MCPDatasetScopeError(...)
try:
return UUID(identifier)
except ValueError as ex:
raise MCPDatasetScopeError(...) from ex
```
Non-string role keys are rejected separately. UUID objects are intentionally
supported, not malformed input. 5fa47d9bcb5975f9d3455e476b892ff4dab5c88b
additionally corrects two inherited docstrings that repeated the false
string-coercion premise. Startup-validation regressions pass, including under
the strict-mode targeted run. This is a configuration-validation improvement,
not a demonstrated role/capability boundary violation under SECURITY.md.
Resolving.
##########
superset/core/mcp/core_mcp_injection.py:
##########
@@ -210,7 +210,10 @@ def decorator(func: F) -> F:
if protect:
from superset.mcp_service.auth import mcp_auth_hook
- wrapped_func = mcp_auth_hook(func)
+ # Pass the registered (extension-prefixed) name so call-time
+ # checks keyed on tool identity cannot be confused by an
+ # extension tool that shares a base name with a host tool.
+ wrapped_func = mcp_auth_hook(func, tool_name=tool_name)
Review Comment:
Verified as invalid as a defect: extension refusal is the documented routing
contract, not an accidental collision. The closing implementation is
228d10188d4d4634142219fcbcebd150a1b897a5, unchanged at verified head
5fa47d9bcb5975f9d3455e476b892ff4dab5c88b:
```python
tool_name, context_type = _get_prefixed_id_with_context(base_tool_name)
# ...
wrapped_func = mcp_auth_hook(func, tool_name=tool_name)
```
and in `enforce_call_dataset_scope`:
```python
if tool_name not in SCOPED_TOOLS:
raise MCPDatasetScopeError(UNSUPPORTED_TOOL_ERROR)
```
`SCOPED_TOOLS` contains only supported built-in identities. The docs
explicitly include `extension tools` among refused authenticated tools.
Stripping the prefix would incorrectly let arbitrary extension code named
`query_dataset` inherit the built-in's supported status.
`test_unscopable_operations_refuse` passes for
`extensions.acme.demo.query_dataset` and an unknown extension tool. No behavior
change; resolving.
--
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]
