sha174n commented on code in PR #38452:
URL: https://github.com/apache/superset/pull/38452#discussion_r3294602565
##########
tests/unit_tests/sql_lab_test.py:
##########
@@ -329,3 +329,112 @@ def test_get_predicates_for_table(mocker: MockerFixture)
-> None:
dataset.get_sqla_row_level_filters.assert_called_once_with(
include_global_guest_rls=False
)
+
+
+@with_config(
+ {
+ "SQLLAB_PAYLOAD_MAX_MB": 50,
+ "DISALLOWED_SQL_FUNCTIONS": {},
+ "DISALLOWED_SQL_TABLES": {},
+ "SQLLAB_CTAS_NO_LIMIT": False,
+ "SQL_MAX_ROW": 100000,
+ "QUERY_LOGGER": None,
+ "TROUBLESHOOTING_LINK": None,
+ "STATS_LOGGER": MagicMock(),
+ }
+)
+def test_rls_blocks_mutation_on_protected_table(mocker: MockerFixture, app) ->
None:
+ """Mutation on a table with active RLS rules is blocked when
RLS_IN_SQLLAB=True."""
+ from superset.exceptions import SupersetSecurityException
+
+ query = mocker.MagicMock()
+ query.limit = 0
+ query.select_as_cta = False
+ query.catalog = "examples"
+ query.database.allow_dml = True
+ query.database.allow_run_async = False
+ query.database.db_engine_spec = mocker.MagicMock()
+ query.database.db_engine_spec.engine = "postgresql"
+ query.database.db_engine_spec.run_multiple_statements_as_one = False
+ query.database.db_engine_spec.allows_sql_comments = True
+ query.database.get_default_schema_for_query.return_value = "public"
+ query.database.get_default_catalog.return_value = "examples"
+
+ mocker.patch("superset.sql_lab.get_query", return_value=query)
+ mocker.patch("superset.sql_lab.db.session.commit")
+ mocker.patch("superset.sql_lab.db.session.refresh")
+ mocker.patch("superset.sql_lab.results_backend", None)
+ mocker.patch("superset.sql_lab.is_feature_enabled", return_value=True)
+ mocker.patch(
+ "superset.sql_lab.get_predicates_for_table",
+ return_value=["owner_id = 2"],
+ )
+
+ with pytest.raises(SupersetSecurityException):
+ execute_sql_statements(
+ query_id=1,
+ rendered_query="UPDATE secret_table SET customer_name='x' WHERE
owner_id=1",
+ return_results=True,
+ store_results=False,
+ start_time=None,
+ expand_data=False,
+ log_params={},
+ )
+
+
+@with_config(
+ {
+ "SQLLAB_PAYLOAD_MAX_MB": 50,
+ "DISALLOWED_SQL_FUNCTIONS": {},
+ "DISALLOWED_SQL_TABLES": {},
+ "SQLLAB_CTAS_NO_LIMIT": False,
+ "SQL_MAX_ROW": 100000,
+ "QUERY_LOGGER": None,
+ "TROUBLESHOOTING_LINK": None,
+ "STATS_LOGGER": MagicMock(),
+ }
+)
+def test_rls_allows_mutation_on_unprotected_table(mocker: MockerFixture, app)
-> None:
+ """Mutation on a table with no RLS rules is not blocked."""
+ from superset.exceptions import SupersetSecurityException
+
+ query = mocker.MagicMock()
+ query.limit = 0
+ query.select_as_cta = False
+ query.catalog = "examples"
+ query.database.allow_dml = True
+ query.database.allow_run_async = False
+ query.database.db_engine_spec = mocker.MagicMock()
+ query.database.db_engine_spec.engine = "postgresql"
+ query.database.db_engine_spec.run_multiple_statements_as_one = False
+ query.database.db_engine_spec.allows_sql_comments = True
+ query.database.get_default_schema_for_query.return_value = "public"
+ query.database.get_default_catalog.return_value = "examples"
+
+ mocker.patch("superset.sql_lab.get_query", return_value=query)
+ mocker.patch("superset.sql_lab.db.session.commit")
+ mocker.patch("superset.sql_lab.db.session.refresh")
+ mocker.patch("superset.sql_lab.results_backend", None)
+ mocker.patch("superset.sql_lab.is_feature_enabled", return_value=True)
+ mocker.patch(
+ "superset.sql_lab.get_predicates_for_table",
+ return_value=[],
+ )
+ try:
+ execute_sql_statements(
+ query_id=1,
+ rendered_query="UPDATE public_table SET name='x' WHERE id=1",
+ return_results=True,
+ store_results=False,
+ start_time=None,
+ expand_data=False,
+ log_params={},
+ )
+ except SupersetSecurityException:
+ pytest.fail(
+ "SupersetSecurityException should not be raised for unprotected
table"
+ )
+ except Exception: # noqa: BLE001, S110
+ # Other exceptions from incomplete mocks are acceptable; the important
+ # assertion is that SupersetSecurityException was NOT raised.
+ pass
Review Comment:
Fixed in 7cf3f5ea43 — removed the blanket `except Exception` and mocked
`execute_query`/`df_to_records` so unrelated failures now fail the test.
##########
superset/sql/parse.py:
##########
@@ -684,14 +720,21 @@ def is_mutating(self) -> bool:
exp.Alter,
)
- for node_type in mutating_nodes:
- if self._parsed.find(node_type):
+ for node in self._parsed.walk():
+ if isinstance(node, mutating_nodes):
return True
- # depending on the dialect (Oracle, MS SQL) the `ALTER` is parsed as a
+ # depending on the dialect (Oracle, MS SQL) some DML is parsed as a
# command, not an expression - check at root level
- if isinstance(self._parsed, exp.Command) and self._parsed.name ==
"ALTER":
- return True # pragma: no cover
+ if isinstance(self._parsed, exp.Command) and self._parsed.name.upper()
in {
Review Comment:
Already covered: `exp.Copy` is in `mutating_nodes` (line 725) and detected
by the `self._parsed.walk()` loop (line 732). Verified `SQLStatement("COPY
users FROM /", engine='postgres').is_mutating()` returns `True`.
--
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]
