From 34a0f50562634da7a29a441ae4c047688841e1d6 Mon Sep 17 00:00:00 2001 From: Laurin Brechter Date: Thu, 18 Jun 2026 11:09:40 +0200 Subject: [PATCH] fix(mcp): align execute_sql access check with SQL Lab UI The MCP execute_sql tool denied queries based solely on can_access_database(), rejecting users who lack the blanket database_access permission but who own or have datasource_access on every dataset referenced by the query. The SQL Lab web UI allows these users through via security_manager.raise_for_access(..., force_dataset_match=True), so the same query could fail via MCP while succeeding in the UI for the same user. Use raise_for_access in the MCP tool to match the UI's access semantics. Co-Authored-By: Claude Sonnet 4.6 --- .../mcp_service/sql_lab/tool/execute_sql.py | 25 ++++++++++++++++--- .../sql_lab/tool/test_execute_sql.py | 11 +++++++- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/superset/mcp_service/sql_lab/tool/execute_sql.py b/superset/mcp_service/sql_lab/tool/execute_sql.py index 1e3dfddf8663..1902553a505d 100644 --- a/superset/mcp_service/sql_lab/tool/execute_sql.py +++ b/superset/mcp_service/sql_lab/tool/execute_sql.py @@ -37,7 +37,11 @@ ) from superset.errors import SupersetErrorType -from superset.exceptions import OAuth2Error, OAuth2RedirectError +from superset.exceptions import ( + OAuth2Error, + OAuth2RedirectError, + SupersetSecurityException, +) from superset.extensions import event_logger from superset.mcp_service.sql_lab.schemas import ( ColumnInfo, @@ -108,13 +112,28 @@ async def execute_sql(request: ExecuteSqlRequest, ctx: Context) -> ExecuteSqlRes error_type=SupersetErrorType.DATABASE_NOT_FOUND_ERROR.value, ) - if not security_manager.can_access_database(database): + # Mirror the SQL Lab UI's access check (CanAccessQueryValidatorImpl): + # a blanket `database_access` grant is sufficient, but in its + # absence a user who owns or has `datasource_access` on every + # dataset referenced by the SQL is also allowed through. Using + # the narrower can_access_database() check alone here previously + # rejected MCP callers that the web UI would have let through. + try: + security_manager.raise_for_access( + database=database, + sql=request.sql, + schema=request.schema_name, + catalog=request.catalog, + template_params=request.template_params, + force_dataset_match=True, + ) + except SupersetSecurityException as security_exc: await ctx.warning( "Access denied to database: %s" % database.database_name ) return ExecuteSqlResponse( success=False, - error=f"Access denied to database {database.database_name}", + error=security_exc.error.message, error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR.value, ) diff --git a/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py b/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py index e495183281b0..39a69a1dad49 100644 --- a/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py +++ b/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py @@ -363,11 +363,20 @@ async def test_execute_sql_access_denied( self, mock_db, mock_security_manager, mcp_server ): """Test error when user lacks database access.""" + from superset.errors import ErrorLevel, SupersetError, SupersetErrorType + from superset.exceptions import SupersetSecurityException + mock_database = _mock_database() mock_db.session.query.return_value.filter_by.return_value.first.return_value = ( mock_database ) - mock_security_manager.can_access_database.return_value = False + mock_security_manager.raise_for_access.side_effect = SupersetSecurityException( + SupersetError( + message="Access denied to database test_db", + error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR, + level=ErrorLevel.ERROR, + ) + ) request = { "database_id": 1,