LaurinBrechter opened a new pull request, #41196: URL: https://github.com/apache/superset/pull/41196
### SUMMARY The MCP `execute_sql` tool gated access purely on `security_manager.can_access_database()`, which only passes for users with the blanket `database_access` permission (or `all_database_access`/`all_datasource_access`). The SQL Lab web UI uses a richer check — `security_manager.raise_for_access(..., force_dataset_match=True)` — which additionally allows a user through if they own, or have `datasource_access` on, every Superset dataset referenced by the query, even without the blanket database-level grant. This meant the same user, running the same query against the same database, could succeed in the SQL Lab UI but get `Access denied to database X` through the MCP `execute_sql` tool — purely because the MCP tool used a narrower check than the UI. This PR switches the MCP tool's access check to call `raise_for_access(database=..., sql=..., schema=..., catalog=..., template_params=..., force_dataset_match=True)`, matching `CanAccessQueryValidatorImpl` (the validator the SQL Lab UI command path uses), so MCP and the UI enforce the same access semantics for the same user. ### RELATED WORK This was found while investigating the false-positive disallowed-function bug fixed in #40963 (different bug, same `execute_sql` code path) — debugging a real-world MCP query failure against a StarRocks-backed database surfaced this separate, pre-existing access-check inconsistency between MCP and the SQL Lab UI. ### FOLLOW-UP (not in this PR) The unified `Database.execute()` API (`superset/sql/execution/executor.py`, `QueryExecutor._check_security()`) that MCP's `execute_sql` calls into has no equivalent of `raise_for_access`/dataset-fallback access check at all — it only checks disallowed functions/tables and DML permission. This PR only patches the MCP tool's own pre-check, which closes the immediate bug, but any other future caller of the unified `database.execute()` API (other MCP tools, new REST endpoints, etc.) could hit the same gap. Moving the `raise_for_access` call into the shared executor itself, so every caller of the unified API gets the same security semantics as SQL Lab automatically, would be a more general fix — but it's a larger, more invasive change to shared security-sensitive code and is left as a separate follow-up. ### TESTING INSTRUCTIONS - Updated `tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py::test_execute_sql_access_denied` to mock `raise_for_access` raising `SupersetSecurityException` instead of `can_access_database` returning `False`. - Manual repro: grant a user `datasource_access` (or ownership) on a dataset in a schema, but withhold blanket `database_access` on the parent database connection. Run the dataset's query via SQL Lab UI (succeeds) and via MCP `execute_sql` (previously failed with "Access denied to database X", now succeeds). ### ADDITIONAL INFORMATION - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration - [ ] Introduces new feature or API - [ ] Removes existing feature or API -- 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]
