From 376bfd05bdba2bbc4bde2d209324105d0d408ee4 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Mon, 18 Mar 2024 15:38:58 -0400 Subject: [PATCH] fix: pass valid SQL to SM (#27464) --- superset/commands/base.py | 2 +- superset/security/manager.py | 6 +++- tests/unit_tests/commands/dataset/__init__.py | 16 +++++++++ tests/unit_tests/security/manager_test.py | 35 +++++++++++++++++++ 4 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 tests/unit_tests/commands/dataset/__init__.py diff --git a/superset/commands/base.py b/superset/commands/base.py index 8b330d0669..d2efcda4fe 100644 --- a/superset/commands/base.py +++ b/superset/commands/base.py @@ -58,7 +58,7 @@ class CreateMixin: # pylint: disable=too-few-public-methods return populate_owner_list(owner_ids, default_to_user=True) -class UpdateMixin: # pylint: disable=too-few-public-methods +class UpdateMixin: @staticmethod def populate_owners(owner_ids: Optional[list[int]] = None) -> list[User]: """ diff --git a/superset/security/manager.py b/superset/security/manager.py index d56c0ad688..94719cb524 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -59,6 +59,7 @@ from superset.exceptions import ( DatasetInvalidPermissionEvaluationException, SupersetSecurityException, ) +from superset.jinja_context import get_template_processor from superset.security.guest_token import ( GuestToken, GuestTokenResources, @@ -1956,11 +1957,14 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods return if query: + # make sure the quuery is valid SQL by rendering any Jinja + processor = get_template_processor(database=query.database) + rendered_sql = processor.process_template(query.sql) default_schema = database.get_default_schema_for_query(query) tables = { Table(table_.table, table_.schema or default_schema) for table_ in sql_parse.ParsedQuery( - query.sql, + rendered_sql, engine=database.db_engine_spec.engine, ).tables } diff --git a/tests/unit_tests/commands/dataset/__init__.py b/tests/unit_tests/commands/dataset/__init__.py new file mode 100644 index 0000000000..13a83393a9 --- /dev/null +++ b/tests/unit_tests/commands/dataset/__init__.py @@ -0,0 +1,16 @@ +# 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. diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index 325531c25b..22ec0dda4a 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -27,6 +27,7 @@ from superset.exceptions import SupersetSecurityException from superset.extensions import appbuilder from superset.models.slice import Slice from superset.security.manager import SupersetSecurityManager +from superset.sql_parse import Table from superset.superset_typing import AdhocMetric from superset.utils.core import override_user @@ -245,6 +246,40 @@ def test_raise_for_access_query_default_schema( ) +def test_raise_for_access_jinja_sql(mocker: MockFixture, app_context: None) -> None: + """ + Test that Jinja gets rendered to SQL. + """ + sm = SupersetSecurityManager(appbuilder) + mocker.patch.object(sm, "can_access_database", return_value=False) + mocker.patch.object(sm, "get_schema_perm", return_value="[PostgreSQL].[public]") + mocker.patch.object(sm, "can_access", return_value=False) + mocker.patch.object(sm, "is_guest_user", return_value=False) + get_table_access_error_object = mocker.patch.object( + sm, "get_table_access_error_object" + ) + SqlaTable = mocker.patch("superset.connectors.sqla.models.SqlaTable") + SqlaTable.query_datasources_by_name.return_value = [] + + database = mocker.MagicMock() + database.get_default_schema_for_query.return_value = "public" + query = mocker.MagicMock() + query.database = database + query.sql = "SELECT * FROM {% if True %}ab_user{% endif %} WHERE 1=1" + + with pytest.raises(SupersetSecurityException): + sm.raise_for_access( + database=None, + datasource=None, + query=query, + query_context=None, + table=None, + viz=None, + ) + + get_table_access_error_object.assert_called_with({Table("ab_user", "public")}) + + def test_raise_for_access_chart_for_datasource_permission( mocker: MockFixture, app_context: None,