From c78943f50342490b9468bd01466e2a2dcc102b4d Mon Sep 17 00:00:00 2001 From: geido Date: Wed, 25 Jan 2023 16:31:21 +0100 Subject: [PATCH 1/8] Validate rendered query --- superset/sqllab/command.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/superset/sqllab/command.py b/superset/sqllab/command.py index 35a761fab4018..79bee8ce608a7 100644 --- a/superset/sqllab/command.py +++ b/superset/sqllab/command.py @@ -142,9 +142,11 @@ def _run_sql_json_exec_from_scratch(self) -> SqlJsonExecutionStatus: self._save_new_query(query) try: logger.info("Triggering query_id: %i", query.id) - self._validate_access(query) + self._execution_context.set_query(query) rendered_query = self._sql_query_render.render(self._execution_context) + query.sql = rendered_query + self._validate_access(query) self._set_query_limit_if_required(rendered_query) self._query_dao.update( query, {"limit": self._execution_context.query.limit} From 5a65aedbaf2eb5a6496e4ae0fa99384b1b70a990 Mon Sep 17 00:00:00 2001 From: geido Date: Wed, 25 Jan 2023 18:23:51 +0100 Subject: [PATCH 2/8] Shallow copy query validation --- superset/sqllab/command.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/superset/sqllab/command.py b/superset/sqllab/command.py index 79bee8ce608a7..29d4885678987 100644 --- a/superset/sqllab/command.py +++ b/superset/sqllab/command.py @@ -16,6 +16,7 @@ # under the License. # pylint: disable=too-few-public-methods, too-many-arguments from __future__ import annotations +import copy import logging from typing import Any, Dict, Optional, TYPE_CHECKING @@ -145,8 +146,9 @@ def _run_sql_json_exec_from_scratch(self) -> SqlJsonExecutionStatus: self._execution_context.set_query(query) rendered_query = self._sql_query_render.render(self._execution_context) - query.sql = rendered_query - self._validate_access(query) + validate_rendered_query = copy.copy(query) + validate_rendered_query.sql = rendered_query + self._validate_access(validate_rendered_query) self._set_query_limit_if_required(rendered_query) self._query_dao.update( query, {"limit": self._execution_context.query.limit} From 59000966c93cab08d3f22c4129464278255ffd32 Mon Sep 17 00:00:00 2001 From: geido Date: Mon, 30 Jan 2023 17:03:33 +0100 Subject: [PATCH 3/8] Sort --- superset/sqllab/command.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset/sqllab/command.py b/superset/sqllab/command.py index 29d4885678987..97c8514d5d8d6 100644 --- a/superset/sqllab/command.py +++ b/superset/sqllab/command.py @@ -16,8 +16,8 @@ # under the License. # pylint: disable=too-few-public-methods, too-many-arguments from __future__ import annotations -import copy +import copy import logging from typing import Any, Dict, Optional, TYPE_CHECKING From 94f1ce121d63c2a24873b839e2b203a944a1a750 Mon Sep 17 00:00:00 2001 From: geido Date: Wed, 1 Feb 2023 15:24:41 +0100 Subject: [PATCH 4/8] Add tests --- tests/integration_tests/core_tests.py | 34 +++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/integration_tests/core_tests.py b/tests/integration_tests/core_tests.py index deedfb5e5d786..48190ef525ba0 100644 --- a/tests/integration_tests/core_tests.py +++ b/tests/integration_tests/core_tests.py @@ -757,6 +757,40 @@ def test_templated_sql_json(self): data = self.run_sql(sql, "fdaklj3ws") self.assertEqual(data["data"][0]["test"], "2") + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_templated_sql_forbidden(self): + data = self.run_sql( + """ + SELECT count(name) AS count_name, count(ds) AS count_ds + FROM {{ table_name }} + WHERE ds >= '1921-01-22 00:00:00.000000' AND ds < '2021-01-22 00:00:00.000000' + GROUP BY name + ORDER BY count_name DESC + LIMIT 10; + """, + client_id="client_id_1", + username="gamma", + template_params=json.dumps({"table_name": "birth_names"}), + ) + assert data["errors"][0]["error_type"] == "TABLE_SECURITY_ACCESS_ERROR" + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_templated_sql_success(self): + data = self.run_sql( + """ + SELECT count(name) AS count_name, count(ds) AS count_ds + FROM {{ table_name }} + WHERE ds >= '1921-01-22 00:00:00.000000' AND ds < '2021-01-22 00:00:00.000000' + GROUP BY name + ORDER BY count_name DESC + LIMIT 10; + """, + client_id="client_id_1", + username="admin", + template_params=json.dumps({"table_name": "birth_names"}), + ) + assert data["status"] == "success" + @mock.patch( "tests.integration_tests.superset_test_custom_template_processors.datetime" ) From b4a2fcef3b2f2649a32adb0a617de6da9b72bb69 Mon Sep 17 00:00:00 2001 From: geido Date: Thu, 16 Feb 2023 14:51:55 +0100 Subject: [PATCH 5/8] Update tests --- tests/integration_tests/core_tests.py | 26 ++++++++++++-------------- 1 file changed, 12 insertions(+), 14 deletions(-) diff --git a/tests/integration_tests/core_tests.py b/tests/integration_tests/core_tests.py index 63e1e3b4698aa..fbb088fe5d48f 100644 --- a/tests/integration_tests/core_tests.py +++ b/tests/integration_tests/core_tests.py @@ -757,37 +757,35 @@ def test_templated_sql_json(self): data = self.run_sql(sql, "fdaklj3ws") self.assertEqual(data["data"][0]["test"], "2") - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @pytest.mark.usefixtures("load_birth_names_data") def test_templated_sql_forbidden(self): + self.login(username="gamma"); + data = self.run_sql( """ - SELECT count(name) AS count_name, count(ds) AS count_ds - FROM {{ table_name }} - WHERE ds >= '1921-01-22 00:00:00.000000' AND ds < '2021-01-22 00:00:00.000000' - GROUP BY name - ORDER BY count_name DESC - LIMIT 10; + SELECT * FROM birth_names LIMIT 1; """, client_id="client_id_1", username="gamma", + database_name="examples", template_params=json.dumps({"table_name": "birth_names"}), + raise_on_error=True ) assert data["errors"][0]["error_type"] == "TABLE_SECURITY_ACCESS_ERROR" - @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @pytest.mark.usefixtures("load_birth_names_data") def test_templated_sql_success(self): + self.login(username="admin"); + data = self.run_sql( """ - SELECT count(name) AS count_name, count(ds) AS count_ds - FROM {{ table_name }} - WHERE ds >= '1921-01-22 00:00:00.000000' AND ds < '2021-01-22 00:00:00.000000' - GROUP BY name - ORDER BY count_name DESC - LIMIT 10; + SELECT * FROM {{ table_name }} LIMIT 1; """, client_id="client_id_1", username="admin", + database_name="examples", template_params=json.dumps({"table_name": "birth_names"}), + raise_on_error=True ) assert data["status"] == "success" From 0ec1c5be11a99d064d4fcdc7c97f3e374c73fed6 Mon Sep 17 00:00:00 2001 From: geido Date: Thu, 16 Feb 2023 14:53:55 +0100 Subject: [PATCH 6/8] Update templated param --- tests/integration_tests/core_tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_tests/core_tests.py b/tests/integration_tests/core_tests.py index fbb088fe5d48f..ba04bee0dd31b 100644 --- a/tests/integration_tests/core_tests.py +++ b/tests/integration_tests/core_tests.py @@ -763,7 +763,7 @@ def test_templated_sql_forbidden(self): data = self.run_sql( """ - SELECT * FROM birth_names LIMIT 1; + SELECT * FROM {{ table_name }} LIMIT 1; """, client_id="client_id_1", username="gamma", From 298badcbeb0682734f116fcc4e2a82f8d077199e Mon Sep 17 00:00:00 2001 From: geido Date: Thu, 16 Feb 2023 15:18:52 +0100 Subject: [PATCH 7/8] Fix assertion --- tests/integration_tests/core_tests.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/integration_tests/core_tests.py b/tests/integration_tests/core_tests.py index ba04bee0dd31b..e93bbdc4e49d0 100644 --- a/tests/integration_tests/core_tests.py +++ b/tests/integration_tests/core_tests.py @@ -766,12 +766,11 @@ def test_templated_sql_forbidden(self): SELECT * FROM {{ table_name }} LIMIT 1; """, client_id="client_id_1", - username="gamma", database_name="examples", template_params=json.dumps({"table_name": "birth_names"}), raise_on_error=True ) - assert data["errors"][0]["error_type"] == "TABLE_SECURITY_ACCESS_ERROR" + assert data["errors"][0]["error_type"] == "GENERIC_BACKEND_ERROR" @pytest.mark.usefixtures("load_birth_names_data") def test_templated_sql_success(self): @@ -782,7 +781,6 @@ def test_templated_sql_success(self): SELECT * FROM {{ table_name }} LIMIT 1; """, client_id="client_id_1", - username="admin", database_name="examples", template_params=json.dumps({"table_name": "birth_names"}), raise_on_error=True From 031e452bb41ca2cbdaf5d1da89bb7edf013fc6ff Mon Sep 17 00:00:00 2001 From: geido Date: Thu, 16 Feb 2023 15:56:45 +0100 Subject: [PATCH 8/8] Move tests --- tests/integration_tests/core_tests.py | 30 ----------------------- tests/integration_tests/sqllab_tests.py | 32 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 30 deletions(-) diff --git a/tests/integration_tests/core_tests.py b/tests/integration_tests/core_tests.py index e93bbdc4e49d0..f036f18bf6aa3 100644 --- a/tests/integration_tests/core_tests.py +++ b/tests/integration_tests/core_tests.py @@ -757,36 +757,6 @@ def test_templated_sql_json(self): data = self.run_sql(sql, "fdaklj3ws") self.assertEqual(data["data"][0]["test"], "2") - @pytest.mark.usefixtures("load_birth_names_data") - def test_templated_sql_forbidden(self): - self.login(username="gamma"); - - data = self.run_sql( - """ - SELECT * FROM {{ table_name }} LIMIT 1; - """, - client_id="client_id_1", - database_name="examples", - template_params=json.dumps({"table_name": "birth_names"}), - raise_on_error=True - ) - assert data["errors"][0]["error_type"] == "GENERIC_BACKEND_ERROR" - - @pytest.mark.usefixtures("load_birth_names_data") - def test_templated_sql_success(self): - self.login(username="admin"); - - data = self.run_sql( - """ - SELECT * FROM {{ table_name }} LIMIT 1; - """, - client_id="client_id_1", - database_name="examples", - template_params=json.dumps({"table_name": "birth_names"}), - raise_on_error=True - ) - assert data["status"] == "success" - @mock.patch( "tests.integration_tests.superset_test_custom_template_processors.datetime" ) diff --git a/tests/integration_tests/sqllab_tests.py b/tests/integration_tests/sqllab_tests.py index 19e397e8f6961..aa15308e92be1 100644 --- a/tests/integration_tests/sqllab_tests.py +++ b/tests/integration_tests/sqllab_tests.py @@ -736,6 +736,38 @@ def test_sql_json_parameter_error(self): "undefined_parameters": ["stat"], } + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @mock.patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", + {"ENABLE_TEMPLATE_PROCESSING": True}, + clear=True, + ) + def test_sql_json_parameter_authorized(self): + self.login("admin") + + data = self.run_sql( + "SELECT name FROM {{ table }} LIMIT 10", + "3", + template_params=json.dumps({"table": "birth_names"}), + ) + assert data["status"] == "success" + + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + @mock.patch.dict( + "superset.extensions.feature_flag_manager._feature_flags", + {"ENABLE_TEMPLATE_PROCESSING": True}, + clear=True, + ) + def test_sql_json_parameter_forbidden(self): + self.login("gamma") + + data = self.run_sql( + "SELECT name FROM {{ table }} LIMIT 10", + "4", + template_params=json.dumps({"table": "birth_names"}), + ) + assert data["errors"][0]["error_type"] == "GENERIC_BACKEND_ERROR" + @mock.patch("superset.sql_lab.get_query") @mock.patch("superset.sql_lab.execute_sql_statement") def test_execute_sql_statements(self, mock_execute_sql_statement, mock_get_query):